[WIP] Update effects for acquire and release operations#8399
[WIP] Update effects for acquire and release operations#8399
Conversation
One of the primary use cases of `EffectAnalyzer` is to determine whether one expression (or a sequence of expressions) can be reordered before or after another expression (or sequence of expressions). Until now, the check whether effects "invalidate" each other and would prevent reordering has been symmetrical, checking read-write, write-write, and write-read conflicts as well as symmetric conditions that would prevent reordering. But acquire load and release store operations break this symmetry. Consider an expression A containing and an acquire load (and no other effects) and an expression B containing a release store (and no other effects), where we know the accesses do not alias. Then if A is before B, it is not valid to reorder the expressions to have B before A because the acquire load cannot move forward past the release store and the release store cannot move back past the acquire load. However, if B is before A, it _is_ valid to reorder them, since memory accesses are generally allowed to move forward past acquire loads and back past release stores. Update `EffectAnalyzer` to take these allowed reorderings into account by tracking the strictest memory orders used to read or write to shared locations in the analyzed expressions. Also update it to differentiate between shared and unshared memories, structs, and arrays to account for both the fact that accesses to shared and unshared locations can never alias and the fact that accesses to unshared locations can never participate in synchronization. While we're at it, treat effects due to e.g. nullability of reference parameters and mutability of fields more uniformly across expressions. Now that the effect comparison to see whether reordering is prevented is no longer symmetric, rename it from `invalidates` to the more descriptive `orderedBefore`, where `A.orderedBefore(B)` returns true iff `A` ocurring before `B` implies that there is an ordering constraint that would prevent reordering `A` and `B` past each other. To accomodate users trying to move expressions in either direction, also add an `A.orderedAfter(B)` method for the case where `A` occurs after `B`. WIP because I need to add more testing for the effect analysis itself and for the passes to ensure they are performing effect comparisons in the correct directions.
| if (effects.hasExternalBreakTargets()) { | ||
| o << "hasExternalBreakTargets\n"; | ||
| } | ||
| o << "}"; |
There was a problem hiding this comment.
The { on line 23 will be lonely after this
| // The strongest memory orders used to read and write to any shared location | ||
| // (e.g. shared memories, shared GC structs, etc), if any. | ||
| MemoryOrder readOrder = MemoryOrder::Unordered; | ||
| MemoryOrder writeOrder = MemoryOrder::Unordered; |
There was a problem hiding this comment.
This adds a lot of fields to this core data structure - please test if it does not have a performance cost. One thing we might do is use : 1 for all the bools above, to avoid growing this structure by a lot (several passes store many copies).
| // Changes something in globally-stored state. | ||
| bool writesGlobalState() const { | ||
| return globalsWritten.size() || writesMemory || writesTable || | ||
| writesStruct || writesArray || isAtomic || calls; |
There was a problem hiding this comment.
Was isAtomic removed intentionally? I guess it moved into hasSynchronization?
There was a problem hiding this comment.
Yes, isAtomic is intentionally removed.
| parent.writeOrder = std::max(parent.writeOrder, curr->order); | ||
| } else { | ||
| parent.writesMemory = true; | ||
| } |
There was a problem hiding this comment.
can perhaps use a helper for this repeating pattern? void writesMemory(Name memory, Order order); etc.
| // We don't want this to be moved out of loops, but it doesn't otherwises | ||
| // matter much how it gets reordered. Say we transfer control as a coarse | ||
| // approximation of this. | ||
| parent.branchesOut = true; |
There was a problem hiding this comment.
This could be in an earlier PR perhaps? Just for bisection purposes later.
| parent.readsSharedMemory = true; | ||
| } else { | ||
| parent.readsMemory = true; | ||
| } |
There was a problem hiding this comment.
This pattern also repeats a lot
| EffectAnalyzer aEffects(passOptions, module, a); | ||
| EffectAnalyzer bEffects(passOptions, module, b); | ||
| return !aEffects.invalidates(bEffects); | ||
| return !aEffects.orderedBefore(bEffects); |
There was a problem hiding this comment.
Shouldn't this check the other direction too? orderedBefore assumes a is before b, but this function doesn't. Or is the intention for this to assume that ordering as well? The comment could be more explicit about that, then?
There was a problem hiding this comment.
Yes, this should also assume an initial ordering.
2 participants
One of the primary use cases of
EffectAnalyzeris to determine whetherone expression (or a sequence of expressions) can be reordered before or
after another expression (or sequence of expressions). Until now, the
check whether effects "invalidate" each other and would prevent
reordering has been symmetrical, checking read-write, write-write, and
write-read conflicts as well as symmetric conditions that would prevent
reordering. But acquire load and release store operations break this
symmetry.
Consider an expression A containing an acquire load (and no other
effects) and an expression B containing a release store (and no other
effects), where we know the accesses do not alias. Then if A is before
B, it is not valid to reorder the expressions to have B before A because
the acquire load cannot move forward past the release store and the
release store cannot move back past the acquire load. However, if B is
before A, it is valid to reorder them, since memory accesses are
generally allowed to move forward past acquire loads and back past
release stores.
Update
EffectAnalyzerto take these allowed reorderings into accountby tracking the strictest memory orders used to read or write to shared
locations in the analyzed expressions. Also update it to differentiate
between shared and unshared memories, structs, and arrays to account for
both the fact that accesses to shared and unshared locations can never
alias and the fact that accesses to unshared locations can never
participate in synchronization. While we're at it, treat effects due to
e.g. nullability of reference parameters and mutability of fields more
uniformly across expressions.
Now that the effect comparison to see whether reordering is prevented is
no longer symmetric, rename it from
invalidatesto the moredescriptive
orderedBefore, whereA.orderedBefore(B)returns true iffAocurring beforeBimplies that there is an ordering constraintthat would prevent reordering
AandBpast each other. To accomodateusers trying to move expressions in either direction, also add an
A.orderedAfter(B)method for the case whereAoccurs afterB.WIP because I need to add more testing for the effect analysis itself
and for the passes to ensure they are performing effect comparisons in
the correct directions.