Alias analysis: Make "observed" state per-instruction and flow insensitive

[fitzgen]

We were previously computing "observed" state per-region, and doing this in a
flow-sensitive manner. Observation flows backwards from observers to the
observed memory state's last mutator, however our LastStores is a forwards
flow-sensitive analysis, so its results for observation were not sound.

This commit switches observation from being per-region and flow sensitive, to
being per-store instruction and flow insensitive. The set of observed store
instructions is computed up front in the existing fixed point that initializes
each block's initial LastStores. Dead-store elimination guards on its
candidate store not being in the observed-stores set. LastStores::meet_from
adds the last store instruction from each CFG predecessor to the observed-stores
set, which is what fixes the incorrect DSE of a store inside a loop from outside
that loop in #13990.

Fixes #13990

[cfallin]

I think there may be a simpler approach: when we come to a back-edge, all stores in the carried flow-sensitive state are observed. That, too, (i) allows loop-free (within-loop) dead store elimination, (ii) fixes the soundness issue with a conservative approximation (but not too conservative), (iii) is single-pass. It's conceptually simpler, IMHO: rather than reasoning about where an analysis might be unsound post-hoc ("the conditions are otherwise met but they cross this kind of edge"), it covers the gap exactly where it comes in (edge to a block we've already analyzed and won't visit again, due to non-fix-point --> assume the worst).

And actually I think there's a related fixpoint soundness issue here that doesn't require loops at all:

        A
  /          \
C            D
 |  \      / |
 |   \   /   |
 |     X     |
 |  /     \  |
 E           F

in this CFG, we might process in order A, C, E, D, F; when processing D, we have the edge D->E but we've already processed E, so we won't re-process it and potentially mark stores in D as observed.

So I think we need to actually take any edge to a block we've already processed as a conservative "observes every store" edge. That both solves the loop issue in #13990 and addresses the CFG here. What do you think?

[fitzgen]

when we come to a back-edge, all stores in the carried flow-sensitive state are observed.

The problem with this approach is that it involves large changes to the way that AliasAnalysis operates and integrates with the egraphs pass. Right now, it makes its changes online, as we analyze each instruction. By the time we see a back edge, and retroactively mark all the stores as observed, it's already too late because the rewrites happened already (the "dead" stores have already been removed).

[cfallin]

I don't think I understand: how is "this edge observes all stores in the memory state" different from "the forward scan suddenly encountered lots of loads"? In other words, we are not going back and editing anything; we are setting a bit on downward-exposed stores, exactly those which have not yet encountered a possibly-overwriting store.

[cfallin]

(I believe it's also necessary in the presence of the CFG I drew above -- even without loops)

[cfallin]

A few notes I had forgotten to add here after discussing this offline with Nick yesterday:

• We realized that another core issue is that the observed-store state is forward-carried flow-sensitive state. That creates issues because discovered observation bits have to somehow propagate back upward -- it's fundamentally a backward-flowing property.
• The postdom check kind of patches over some cases here (e.g. a divergent return path that also observes) but there are still propagation issues.
• The fix seems to be what I suggested above, observe-on-edge-to-already-processed-block (this is fine because it sees the state before it reaches any store that might overwrite the original -- just like a load would), plus making the observed bits non-flow-sensitive.
• Also, we need to "observe" any store that leaves a last-store slot, since the slot is may-alias not must-alias (does not summarize all stores to that abstract region).

[cfallin]

Thanks for the update -- this looks right to me.

The "don't actually observe at update to same region" bit is really subtle -- basically the postdom check plus forward-flow combine to ensure that at possible overwriters, we see all outbound paths from the original store (there are no escaping/early-returning paths), so we can rely on any partial-overwrite causing a different last-store at that point. The case I had been imagining was where we had a (conflated to same coarse region) not-really-overwriter on a path to a return, so original store was still exposed; but if there is any path to an early return, later store won't postdom earlier store anyway. So 👍

[cfallin]

(Merge conflict before this can merge)