[PATCH] D49425: [MemorySSAUpdater] Update Phi operands after trivial Phi elimination

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this!

Looks like the MSSA that we're starting with here has a redundant Phi to start with (`6 = MemoryPhi({_, liveOnEntry}, {_2, 6})`), so it's unsurprising that we try to remove it. In general, I'm noticing that MSSA tries really hard to keep this nice minimal Phi form, but it's really easy for a user to inadvertently make a Phi trivially removable with a RAUW or similar. Fixing this is likely nontrivial + out of the scope of this patch, so I'm happy to support this for now. It's just a mildly sad state of affairs. :)

> Do we ever reach the PHIExistsButNeedsUpdate case? Is it when a Phi existed beforehand, meaning we did not create it? I can't think of another way to reach that state.

Good point. In general, we'd hit that case if we called `getPreviousDefRecursive` on a BB with > 1 pred with an existing Phi, but I don't see how that can happen today. The only callers are:

- `getPreviousDef`, which, unless it's given a Phi, is guaranteed to not call `getPreviousDefRecursive` if there's a Phi in the same BB as its argument.
- `getPreviousDefFromEnd`, which has an identical guarantee (except it takes a BB instead of a MemoryAccess)

...And we only ever call `getPreviousDef` with Uses/Defs.

Replacing that code with `llvm_unreachable` gave 0 errors in a clang bootstrap, so I assume it's dead code. If you'd like to remove it (or replace it with an assertion), please do so in a separate patch.

> SmallVector<WeakVH, 8> PhiOps fixes the bug on its own (without the rest changes) and I am wondering why..

Best guess: we end up calling `tryRemoveTrivialPhi(nullptr, VectorOfWeakVH)`, which ends up seeing a vector consisting of `{nullptr, liveOnEntry}` (nullptr being a special value we use in `tryRemoveTrivialPhi`, so passing it in as an operand is really sketchy), which then "recurses" on and returns `liveOnEntry`. `liveOnEntry != nullptr`, so we don't try to create a phi/etc.

I doubt all of that is right, but ...

> When we mark a block as visited why do we cache it? When the recursion ends we might trivially remove the Phi. In that case the second cache insertion for the same key block should fail, no?

Not sure I understand the question. Do you mean "why do we cache the Phi we've just created if we're visiting a block for the second time?" If so, the cache will automatically track the removal if we do ultimately end up removing the phi. If not, we'll fill the Phi in when we wind our stack back to the bit that populates it, so the second cache insertion should be a nop anyway..

> Interestingly enough the reproducer only made opt crash in bitcode form and not in IR form.

Does the original test-case crash reliably as IR for you? If so, please use that instead. (Phab won't let me download the attached bitcode, but with asan, I see use-after-free crashes 100% of the time in the original repro).



================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:68
     // Mark us visited so we can detect a cycle
-    SmallVector<MemoryAccess *, 8> PhiOps;
+    SmallVector<WeakVH, 8> PhiOps;
 
----------------
Should this be a `TrackingVH<MemoryAccess>` instead? That way, we don't need the "Phi ops may be out of date" loop below.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:90
+      for (auto *Pred : predecessors(BB)) {
+        auto *MA = dyn_cast_or_null<MemoryAccess>(PhiOps[i]);
+        if (!MA)
----------------
`cast_or_null`, please. We know this'll be a `MemoryAccess` if it's non-null.


https://reviews.llvm.org/D49425