-
Notifications
You must be signed in to change notification settings - Fork 839
Optimizer support for atomic instructions #1094
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
Changes from all commits
c6668c7
a4e11af
8c56cd3
fd310f5
9540c04
c619356
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -53,12 +53,14 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> { | |
| // (so a trap may occur later or earlier, if it is | ||
| // going to occur anyhow), but we can't remove them, | ||
| // they count as side effects | ||
| bool isAtomic = false; // An atomic load/store/RMW/Cmpxchg or an operator that | ||
| // has a defined ordering wrt atomics (e.g. grow_memory) | ||
|
|
||
| bool accessesLocal() { return localsRead.size() + localsWritten.size() > 0; } | ||
| bool accessesGlobal() { return globalsRead.size() + globalsWritten.size() > 0; } | ||
| bool accessesMemory() { return calls || readsMemory || writesMemory; } | ||
| bool hasSideEffects() { return calls || localsWritten.size() > 0 || writesMemory || branches || globalsWritten.size() > 0 || implicitTrap; } | ||
| bool hasAnything() { return branches || calls || accessesLocal() || readsMemory || writesMemory || accessesGlobal() || implicitTrap; } | ||
| bool hasSideEffects() { return calls || localsWritten.size() > 0 || writesMemory || branches || globalsWritten.size() > 0 || implicitTrap || isAtomic; } | ||
| bool hasAnything() { return branches || calls || accessesLocal() || readsMemory || writesMemory || accessesGlobal() || implicitTrap || isAtomic; } | ||
|
|
||
| // checks if these effects would invalidate another set (e.g., if we write, we invalidate someone that reads, they can't be moved past us) | ||
| bool invalidates(EffectAnalyzer& other) { | ||
|
|
@@ -67,6 +69,12 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> { | |
| || (accessesMemory() && (other.writesMemory || other.calls))) { | ||
| return true; | ||
| } | ||
| // All atomics are sequentially consistent for now, and ordered wrt other | ||
| // memory references. | ||
| if ((isAtomic && other.accessesMemory()) || | ||
| (other.isAtomic && accessesMemory())) { | ||
| return true; | ||
| } | ||
| for (auto local : localsWritten) { | ||
| if (other.localsWritten.count(local) || other.localsRead.count(local)) { | ||
| return true; | ||
|
|
@@ -176,10 +184,24 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> { | |
| } | ||
| void visitLoad(Load *curr) { | ||
| readsMemory = true; | ||
| isAtomic |= curr->isAtomic; | ||
| if (!ignoreImplicitTraps) implicitTrap = true; | ||
| } | ||
| void visitStore(Store *curr) { | ||
| writesMemory = true; | ||
| isAtomic |= curr->isAtomic; | ||
| if (!ignoreImplicitTraps) implicitTrap = true; | ||
| } | ||
| void visitAtomicRMW(AtomicRMW* curr) { | ||
| readsMemory = true; | ||
| writesMemory = true; | ||
| isAtomic = true; | ||
| if (!ignoreImplicitTraps) implicitTrap = true; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what's the possible implicit trap here? read memory out of bounds I guess?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Atomics trap if misaligned as well. |
||
| } | ||
| void visitAtomicCmpxchg(AtomicCmpxchg* curr) { | ||
| readsMemory = true; | ||
| writesMemory = true; | ||
| isAtomic = true; | ||
| if (!ignoreImplicitTraps) implicitTrap = true; | ||
| } | ||
| void visitUnary(Unary *curr) { | ||
|
|
@@ -219,11 +241,16 @@ struct EffectAnalyzer : public PostWalker<EffectAnalyzer> { | |
| } | ||
| } | ||
| void visitReturn(Return *curr) { branches = true; } | ||
| void visitHost(Host *curr) { calls = true; } | ||
| void visitHost(Host *curr) { | ||
| calls = true; | ||
| // grow_memory modifies the set of valid addresses, and thus can be modeled as modifying memory | ||
| writesMemory = true; | ||
| // Atomics are also sequentially consistent with grow_memory. | ||
| isAtomic = true; | ||
| } | ||
| void visitUnreachable(Unreachable *curr) { branches = true; } | ||
| }; | ||
|
|
||
| } // namespace wasm | ||
|
|
||
| #endif // wasm_ast_effects_h | ||
|
|
||
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 well enough. Would an atomic load be marked as having side effects here?
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.
The idea is to prevent atomic loads from being reordered past each other. With just one thread, loads can be reordered as long as they don't pass a write, but with sequentially-consistent atomic loads, the intervening write could be on another thread. In general I'm trying to be conservative in this patch, but we'll have to stay pretty conservative as long as all atomics are SC.
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.
Makes sense.