Acquire/release memory ordering for loads #8169
Conversation
stevenfontanella
force-pushed
the
relaxed-atomics-cp
branch
from
d634a16 to
39cd787
Compare
stevenfontanella
force-pushed
the
relaxed-atomics-cp
branch
15 times, most recently
from
5e2de8f to
b92a217
Compare
stevenfontanella
force-pushed
the
relaxed-atomics-cp
branch
from
b92a217 to
1bd821c
Compare
There was a problem hiding this comment.
Will update this to match the structure that we chatted about. In the meantime, this should cover all of the logic from this PR.
There was a problem hiding this comment.
It would be great to port this test to lit+FileCheck as a preliminary PR. scripts/port_passes_tests_to_lit.py can help.
| void printMemoryOrder(MemoryOrder order) { | ||
|
|
||
| void | ||
| printMemoryOrder(MemoryOrder order, bool prependSpace, bool appendSpace) { |
There was a problem hiding this comment.
I think we should be able to update the callers of printMemoryOrder so it can just unconditionally prepend the space if it prints anything and never append a space afterward. That would be much simpler.
There was a problem hiding this comment.
it can just unconditionally prepend the space if it prints anything
Do you mean to only prepend a space if something was printed, or to always prepend a space no matter what? I think the former doesn't work because here for example, we need a space between the memory ordering and the heap type name, but if printMemoryOrder didn't print anything, then the space is not needed, and we can't tell which one it is from the caller.
I think it would work to always prepend a space even for empty cases, and then only append a space for acqrel. It's just a little strange that it prints an empty space in the case of seqcst and unordered. Would that way be better?
There was a problem hiding this comment.
Do you mean to only prepend a space if something was printed, or to always prepend a space no matter what?
Only if we will print something. In the code you linked to, you would have to remove the spaces that are already printed before the memory ordering and newly print a space afterward. That space afterward will be the only space separating the heap type name from either the instruction name or the memory order, depending on what was last printed.
There was a problem hiding this comment.
It would also be good to check basic parsing and printing and round-tripping in a new test in test/lit/basic. See e.g. test/lit/basic/exact-imports.wast for the standard RUN lines.
Once it is ready, landing the spec test here in test/spec is good, but as a follow-up at some point it would also be good to upstream the test. It should be added to the shared-everything-threads repo, then shared-everything-threads should be added to the list of repos pulled into the testsuite repo. Finally, our testsuite submodule can be updated and we can remove the version of the test in test/spec.
There was a problem hiding this comment.
Yep, upstreaming the spec test is my plan (I spoke with Rezvan about this as well). Thanks for the details! I'll also add the lit tests.
Part of #8165. Adds support for parsing and serializing the text and binary formats for memory orderings for loads. The C and JS apis are left unchanged, they'll be updated for all atomic operations in the same commit later. test/passes was updated using
python3 scripts/auto_update_tests.py wasm-opt.