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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv marked an inline comment as done.
george.burgess.iv added inline comments.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:274
         continue;
+      if (auto *DefUser = dyn_cast<MemoryDef>(U.getUser()))
+        if (DefUser->isOptimized())
----------------
asbirlea wrote:
> 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.
> If we replace that user with MD, we're optimized DefLower to MD, which may not be correct

You're totally correct. `isOptimized()` isn't just bit, though: it should be verifying that `getOptimized()->getID() == SomeCachedIDInTheMemoryDef`. In this case, that test won't pass, so the `MemoryDef` in question should be considered unoptimized.

> Calling resetOptimized will also remove the optimized user. This relates to D56467.

Looking at D56467 again, that sounds like broken verification code more than anything. We lazily invalidate caches for Uses (e.g. a Use's `DefiningAccess`); I don't see why we should treat Defs any differently. Sorry for not catching that earlier.

If we wanna land this change now and fix the verification later, I'm OK with that. But for all intents and purposes, `Use`s that originate from MemoryDefs' OptimizedAccesses shouldn't be treated as `Use`s unless the MemoryDef still reports itself to be optimized.

...If we find ourselves iterating over the `User`s of MemoryDefs often, it may be worth making an iterator that skips invalidated `User`s. IDK whether that's a common use-case (pun intended) or not, though.

> I can split it up if you prefer it.

It's small enough that I'm OK with bundling it. I was just making sure it wasn't related in a way that I was missing


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:319
+           ++Idx) {
+        auto *IDFPhi = cast<MemoryPhi>(InsertedPHIs[Idx]);
+        auto *BBIDF = IDFPhi->getBlock();
----------------
asbirlea wrote:
> 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.
Yup :)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:285
   SmallVector<WeakVH, 8> FixupList(InsertedPHIs.begin(), InsertedPHIs.end());
+  SmallVector<AssertingVH<MemoryPhi>, 4> NewInsertedPHIs;
+  unsigned NewPhiIndex = InsertedPHIs.size();
----------------
Please sink NewInsertedPhis into the `if (Iter == IterEnd) {` block (removing the trivial Phis later on in this function may invalidate them; I haven't reasoned about whether or not this'll actually happen, but I can't see why we'd care about the guarantee that that doesn't happen, so ...)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:344
+  // Optimize trivial phis.
+  for (unsigned Idx = NewPhiIndex, IdxE = InsertedPHIs.size(); Idx < IdxE;
+       ++Idx) {
----------------
Please grab the value for `IdxE` before the `FixupList.empty()` loop above; that can add Phis, and it shouldn't be profitable to try to remove them (or if it is, it'd be nice to have an explicit example of where that can happen...)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58652