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

[Alina Sbirlea via Phabricator via llvm-commits]

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:
> 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
You're right that replacing the Def should make the user unoptimized due to the different IDs. Removing the change, and I'll see if I can find the test that failed the verification in the previous patch. Arbitrary insertions may trigger a verification failure from here as well.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:322
+        for (auto *Pred : predecessors(BBIDF)) {
+          DenseMap<BasicBlock *, TrackingVH<MemoryAccess>> CachedPreviousDef;
+          IDFPhi->addIncoming(getPreviousDefFromEnd(Pred, CachedPreviousDef),
----------------
george.burgess.iv wrote:
> nit: Would it make sense to hoist this out of one (or both?) loops? Seems like sharing work between `getPreviousDefFromEnd` invocations is harmless.
Yes...but I'd rather not do it here. The current code seems convoluted enough that I'd do any refactoring change separate.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:285
   SmallVector<WeakVH, 8> FixupList(InsertedPHIs.begin(), InsertedPHIs.end());
+  SmallVector<AssertingVH<MemoryPhi>, 4> NewInsertedPHIs;
+  unsigned NewPhiIndex = InsertedPHIs.size();
----------------
george.burgess.iv wrote:
> 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 ...)
You're right, I meant to clear it after adding to InsertedPhis, but sinking to reduce its scope is better!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58652