[PATCH] D58652: [MemorySSA] Make insertDef insert corresponding phi nodes.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea marked 6 inline comments as done.
asbirlea added inline comments.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:274
         continue;
+      if (auto *DefUser = dyn_cast<MemoryDef>(U.getUser()))
+        if (DefUser->isOptimized())
----------------
george.burgess.iv wrote:
> It's unclear to me why this is necessary. We keep an `OptimizedID` in `MemoryDefs` to catch these kinds of movements and transparently invalidate the cached optimized Access in those cases. Does that not work here?
> 
> (It also seems sort of unrelated to this patch, unless I'm missing something?)
IIUC, if we have a def, let's call it `DefLower`, that is optimized to `DefBefore` replaced here, then `DefLower` maintains a field: operator 1, pointing to the optimized access), making it a user of `DefBefore`. If we replace that user with MD, we're optimized `DefLower` to `MD`, which may not be correct. Calling resetOptimized will also remove the `optimized` user. This relates to D56467.

Yes, it's unrelated to this patch, I noticed it when making the changes below and added the change. I can split it up if you prefer it.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:319
+           ++Idx) {
+        auto *IDFPhi = cast<MemoryPhi>(InsertedPHIs[Idx]);
+        auto *BBIDF = IDFPhi->getBlock();
----------------
george.burgess.iv wrote:
> This was pretty tricky for me to reason about. If you're 100% certain that all of this is correct, feel free to leave this as is. Otherwise, can we please shuffle these new `InsertedPhis` into an intermediate `SmallVector<AssertingVH<MemoryPhi>, N>`? The idea would be to keep them there, loop over the new SmallVector, and transfer them into `InsertedPhis` after the `for (auto *Pred : predecessors)` loop below.
> 
> Sharing my work anyway:
> 
> `getPreviousDefFromEnd` is interesting here, since it'll *also* do Phi insertion/population/trivial removal.
> 
> I think we should be fine on the population bit: it only ever calls `getPreviousDefFromEnd`, so existing Phis should never be stomped by it.
> 
> Similarly, insertion should be OK, since it only inserts a Phi when we reach a cycle, and the set of Phis should be relatively complete with the ones we've just added.
> 
> Trivial removal is a bit more tricky. You can totally make it recurse on a trivial Phi we've just created. The fun bit is that it only recurses on the *Users* of the value we're replacing a Phi with.  So the question is whether we can end up in a world with a partially-formed Phi (e.g. we add one pred of `IDFPhi`, so `IDFPhi` is now a `User`, and thus a candidate for trivial deletion). I don't *think* that should be a problem, since we should have a complete and minimal (modulo the Phis we're in the process of inserting) set of Phis at this point. Since `getPreviousDefRecursive` guarantees minimality in most cases(*), I don't see how we can be handed a trivial Phi to put in the Incoming list for this. 
> 
> (*) - The other cases result from irreducible control flow. That's technically nonminimal, but AFAICT it's a nonminimal form that we won't trivially eliminate later on, so...
I *think* I followed the guidelines you outlined. All newly inserted phis are kept separate while calling the recursive getPreviousDef, and removing non-minimal phis is only done of these new phis.
Please let me know if I missed anything.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58652/new/

https://reviews.llvm.org/D58652