-
Notifications
You must be signed in to change notification settings - Fork 832
Add feature flag for relaxed atomics #8192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
07df315 to
2f0fc13
Compare
2f0fc13 to
23f7811
Compare
| ;; NOTE: This test was ported using port_passes_tests_to_lit.py and could be cleaned up. | ||
|
|
||
| ;; RUN: foreach %s %t wasm-opt --safe-heap --enable-threads --enable-simd --enable-memory64 -S -o - | filecheck %s | ||
| ;; RUN: foreach %s %t wasm-opt --safe-heap --enable-threads --enable-simd --enable-memory64 --enable-relaxed-atomics -S -o - | filecheck %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than adding relaxed-atomics to each of these files, let's make a new file that enables just relaxed atomics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand, this test wouldn't pass without --enabled-relaxed-atomics because --safe-heap combined with --enable-threads will cause us to generate safe heap methods that use acqrel ordering (which the next PR fixes but we need this PR first). In the next PR #8193 I add an equivalent test without --enable-relaxed-atomics to show that the acqrel methods aren't added. Also we anyway need a test that enables both relaxed atomics and safe heap to show that the safe acqrel methods are added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. LGTM to land this as-is because of the dependencies between PRs. I think ultimately we don't need to have more than one SafeHeap test that enables relaxed-atomics, though. (Also we can make the test file names much shorter now that they are lit tests.)
tlively
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % those comments.
Part of #8165. Add flag for --enable-relaxed atomics and enable it for the existing SafeHeap tests that previously implicitly depended on it. The next PR #8193 disables generation of AcqRel SafeHeap functions when the feature is disabled and adds new tests to cover that case.
Also validate that the feature is enabled when a module includes an AcqRel load operation (currently the only one we support).