Stream: git-wasmtime

Topic: wasmtime / issue #5817 cranelift-interpreter: Implement t...

Wasmtime GitHub notifications bot (Feb 17 2023 at 14:50):

afonso360 opened issue #5817:

:wave: Hey,

Our interpreter implementation is missing the AtomicRMW instruction here.

AtomicRMW usually performs some operation atomically, but we've decided that our interpreter is thread unsafe and should be able to do this non atomically.

This means that the implementation of this instruction is fairly straight forward.

1. We do a load to an address
2. Perform the operation using the pre existing methods on DataValue
3. We store the new value to the same address

I'm hesitant to mark this as E-Easy mostly due to there being a large number of steps in this task, especially getting all the values and addresses from the instruction, but if someone wants to give this a go and has any questions let me know!

Wasmtime GitHub notifications bot (Feb 17 2023 at 14:53):

afonso360 edited issue #5817:

:wave: Hey,

Our interpreter implementation is missing the AtomicRMW instruction here.

AtomicRMW usually performs some operation atomically, but we've decided that our interpreter is thread unsafe and should be able to do this non atomically.

This means that the implementation of this instruction is fairly straight forward.

1. We do a load to an address
2. Perform the operation using the pre existing methods on DataValue
3. We store the new value to the same address

If someone wants to give this a go and has any questions let me know!

Wasmtime GitHub notifications bot (Feb 17 2023 at 14:53):

afonso360 labeled issue #5817:

:wave: Hey,

Our interpreter implementation is missing the AtomicRMW instruction here.

AtomicRMW usually performs some operation atomically, but we've decided that our interpreter is thread unsafe and should be able to do this non atomically.

This means that the implementation of this instruction is fairly straight forward.

1. We do a load to an address
2. Perform the operation using the pre existing methods on DataValue
3. We store the new value to the same address

If someone wants to give this a go and has any questions let me know!

Wasmtime GitHub notifications bot (Feb 17 2023 at 14:55):

afonso360 edited issue #5817:

:wave: Hey,

Our interpreter implementation is missing the AtomicRMW instruction here.

AtomicRMW usually performs some operation atomically, but we've decided that our interpreter is thread unsafe and should be able to do this non atomically.

This means that the implementation of this instruction is fairly straight forward.

1. We do a load to an address
2. Perform the operation using the pre existing methods on DataValue
3. We store the new value to the same address

We already have a pre-existing testsuite for this functionality so, it should just be enabling those tests for the interpreter by adding a test interpret to the top of the file.

If someone wants to give this a go and has any questions let me know!

Wasmtime GitHub notifications bot (Feb 17 2023 at 20:02):

jameysharp labeled issue #5817:

:wave: Hey,

Our interpreter implementation is missing the AtomicRMW instruction here.

AtomicRMW usually performs some operation atomically, but we've decided that our interpreter is thread unsafe and should be able to do this non atomically.

This means that the implementation of this instruction is fairly straight forward.

1. We do a load to an address
2. Perform the operation using the pre existing methods on DataValue
3. We store the new value to the same address

We already have a pre-existing testsuite for this functionality so, it should just be enabling those tests for the interpreter by adding a test interpret to the top of the file.

If someone wants to give this a go and has any questions let me know!

Wasmtime GitHub notifications bot (Feb 21 2023 at 20:18):

jan-justin commented on issue #5817:

Hey @afonso360 :wave: ,

I am a first-time contributor and keen to take a look at it.

I have a question regarding the tests, specifically these tests. Are they meant to be %atomic_rmw_and_i64 or %atomic_rmw_and_i32? The tests panic with the former, but pass with the latter.

Wasmtime GitHub notifications bot (Feb 21 2023 at 22:09):

afonso360 commented on issue #5817:

I have a question regarding the tests, specifically these tests. Are they meant to be %atomic_rmw_and_i64 or %atomic_rmw_and_i32? The tests panic with the former, but pass with the latter.

Wow, those should definitely call atomic_rmw_and_i32, Good catch!

I'm surprised that these runtests didn't fail before.

I suspect we just zero extend all of these values up to i64 and since it's an and operation everything aligns and works correctly? I'm not sure

I think I remember something about our test-runner ignoring function names and just calling the last-defined function sometimes, which would explain this.

I think that used to be how our test runner worked, but IIRC (this was a while ago) this was changed in #4667 and we now require the function names to exist in the testcase.

Wasmtime GitHub notifications bot (Feb 22 2023 at 09:47):

jan-justin commented on issue #5817:

Thank you both for the clarification!

Given the other tests I suspected as much, however, I did not want to make any assumptions.

Wasmtime GitHub notifications bot (Feb 23 2023 at 11:02):

afonso360 closed issue #5817:

:wave: Hey,

Our interpreter implementation is missing the AtomicRMW instruction here.

AtomicRMW usually performs some operation atomically, but we've decided that our interpreter is thread unsafe and should be able to do this non atomically.

This means that the implementation of this instruction is fairly straight forward.

1. We do a load to an address
2. Perform the operation using the pre existing methods on DataValue
3. We store the new value to the same address

We already have a pre-existing testsuite for this functionality so, it should just be enabling those tests for the interpreter by adding a test interpret to the top of the file.

If someone wants to give this a go and has any questions let me know!

Last updated: Oct 23 2024 at 20:03 UTC