Observe all stores we replace in LastStores; keep track of who observed a store - #14080
Conversation
|
Thanks for this -- very subtle bug here. I think I have some concerns about the way in which we're reasoning carefully about divergent blocks, and patching conclusions on top of the core analysis, rather than getting the core analysis to give us the right answer from-first-principles. I worry that (especially given the chain of subtle bugs we've had here) we may miss something else, too; and even if not, it's very subtle and difficult to reason about and maintain. Instead I think the crux of this comes back to this comment that describes why not to do the "store observes last store that it replaces in the abstract state" step I mentioned here (last point). This would resolve the bug in a principled way because, entering any divergent loop, either a still-downward-exposed store meets a trapping op and is observed, or no traps ever occur and it is just an infinite loop (so is truly not observed). Basically we turn the last-store state into a may-alias rather than must-alias kind of state: any store that could actually be the most recent to a given memory location is either observed (so "committed" in some sense -- we won't remove it) or is in the flow-sensitive state of every path outward from it. The current lossy situation creates the hole that we have to plug instead, and I'm not confident that that's simple enough to reason about that we want to go there. A question though: the comment linked above mentions some optimization opportunities that observe-store-we-overwrote-in-abstract-state would miss. Are there examples of that case that we know about? |
If I understand correctly, what you are proposing ultimately just entails removing that comment and observing the last-store instruction for the region at that comment's old location, correct? modified cranelift/codegen/src/alias_analysis.rs
@@ -211,40 +211,17 @@ impl LastStores {
}
// Store instructions: update the last-store information for this
// instruction's alias region, or, if it has no alias region, treat it
// as a fence.
else if opcode.can_store() {
if let Some(memflags) = func.dfg.insts[inst].memflags() {
match func.dfg.mem_flags[memflags].alias_region() {
Some(region) => {
- // NB: The old last-store instruction is *not* observed
- // here, even though this new store instruction may not
- // fully overwrite it. First, a new store in a block
- // does not itself observe an old store in the same
- // block. Second, the old store will never be an
- // optimization candidate again from here on out:
- //
- // * We won't consider it again as we process the rest
- // of this block, as it won't be in the last-store
- // slot anymore.
- //
- // * What if we re-process this block in our initial
- // fixed point loop? That implies this block is a
- // member of a cycle in the CFG, but `meet_from` only
- // propagates a store instruction when all
- // predecessors agree on the same last-store
- // instruction, but the predecessors already won't
- // agree it is the old store since this block (which
- // is on that path and therefore some kind of
- // transitive predecessor) has already overridden it.
- //
- // Therefore, marking the old last-store as observed
- // here is unnecessary (and, in fact, doing so would
- // only inhibit optimization).
+ observe(func, observed_stores, self.regions[region]);
self.regions[region] = inst.into();
// If this store can trap, then we need to observe
// all other alias regions, to ensure that their state
// is preserved in the case that this store traps
// (similar to the `can_trap()` handling above).
//
// This prevents removing the first store in theThat diff does fix #14053 and passes the new tests added in this PR. However, it fails with missed optimizations on 10 of our alias analysis filetests (maybe even more individual test failures than that, since filetests can have multiple functions/checks, but the runner bails on the first failure). Full alias filetest failuresDigging into one of the failures, Today, we expect the first store (of (Note: removing the remaining store requires additional work to "rewind" the However, the patch above marks the first store observed when we replace it with the second one inside I think that, effectively, that patch means we would only ever be able to DSE the last store to a region inside a block, and will never be able to DSE when the maybe-dead store and overwriter are both within the same block, which feels pretty restrictive. |
|
Nick and I discussed briefly offline; to record for posterity:
|
fitzgen
changed the title
LastStores; keep track of who observed a store
2 participants
Fixes #14053