WASM: Atomic load/store#4470
Conversation
|
@Microsoft/chakra-jit I could use some of you for the review |
Cellule
force-pushed
the
wasm/atomics
branch
2 times, most recently
from
b7da005 to
65a14e0
Compare
Cellule
force-pushed
the
wasm/atomics
branch
3 times, most recently
from
f37c4b5 to
7924814
Compare
|
ping |
1 similar comment
|
ping |
| IR::Instr* | ||
| Lowerer::LowerStAtomicsWasm(IR::Instr* instr) | ||
| { | ||
| #ifdef ENABLE_WASM |
There was a problem hiding this comment.
nit: If we know at compile time that we don't have WASM enabled, why do we even have this function defined? Shouldn't we wrap the whole function definition to catch issues at link-time instead of at run-time?
| Assert(IRType_IsNativeInt(dst->GetType())); | ||
|
|
||
| IR::Instr * done = LowerWasmArrayBoundsCheck(instr, dst); | ||
| m_lowererMD.LowerAtomicStore(dst, src1, done); |
There was a problem hiding this comment.
nit: Without knowing that LowerWasmArrayBoundsCheck returns a pointer to an instruction that is after the check, this looks like we're putting the store before the bounds check. Not really a problem if you know what's going on, but maybe could be a little clearer?
There was a problem hiding this comment.
I think you're right. There is something off about that api as a whole.
I created issue #4751 to track cleaning this up.
| // Move src1 to a register of the same type as dst | ||
| IR::RegOpnd* tmpSrc = IR::RegOpnd::New(dst->GetType(), func); | ||
| Lowerer::InsertMove(tmpSrc, src1, insertBeforeInstr); | ||
| if (dst->IsInt64()) |
There was a problem hiding this comment.
nit: Maybe put in an assert to make sure that we don't get float dsts, since the spec and this code doesn't support them yet, but not high-priority.
Cleanup Uses of ArrayBufferView type
Extract atomics operations from TypedArray.cpp in AtomicsOperations.cpp then share the atomics code for TypedArray and Wasm Add encoding of CMPXCHG8B which has 2 dst and 5 sources
Move generated spec test to `chakra_generated`
Cellule
force-pushed
the
wasm/atomics
branch
from
fc40797 to
07b0be8
Compare
3 participants
Implement wasm atomic load/store operations.
Related #3477
This change is