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

[Alexandros Lamprineas via Phabricator via llvm-commits]

labrinea added a comment.

> 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).

It does, but using `opt -S -O3 ./tc_memphi_gvnhoist.ll -enable-gvn-hoist`. Using bugpoint on that command you get the bitcode I uploaded.

> 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..

Imagine we call `getPreviousDefRecursive` on a BB with > 1 pred. Assume there's no Phi yet. We mark the BB as visited and start collecting PhiOps with recursive calls via `getPreviousDefFromEnd`. By the time we reach BB again we create an empty Phi and cache it for the first time (line 62). As all the PhiOps are collected we call `tryRemoveTrivialPhi` which might have replaced the Phi with another MemoryAccess. Then we try to cache that MemoryAccess with the same BB key (line 110), which should return false.

> Should this be a TrackingVH<MemoryAccess> instead? That way, we don't need the "Phi ops may be out of date" loop below.

Works, but still it's not clear to me why. What happens if the PhiOp is a deleted Phi itself? Do we still keep the instance of the dead Phi even though MSSA has deleted the corresponding MemoryAccess? How would this work without the updating loop?


https://reviews.llvm.org/D49425