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

[George Burgess IV via Phabricator via llvm-commits]

george.burgess.iv added a comment.

Thanks for this!



================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:274
         continue;
+      if (auto *DefUser = dyn_cast<MemoryDef>(U.getUser()))
+        if (DefUser->isOptimized())
----------------
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?)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:301
+    auto Iter = MD->getDefsIterator();
+    Iter++;
+    auto IterEnd = MSSA->getBlockDefs(MD->getBlock())->end();
----------------
nit: I think there's a general preference for preincrement when the difference doesn't matter.


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


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:322
+        for (auto *Pred : predecessors(BBIDF)) {
+          DenseMap<BasicBlock *, TrackingVH<MemoryAccess>> CachedPreviousDef;
+          IDFPhi->addIncoming(getPreviousDefFromEnd(Pred, CachedPreviousDef),
----------------
nit: Would it make sense to hoist this out of one (or both?) loops? Seems like sharing work between `getPreviousDefFromEnd` invocations is harmless.


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:341
+  // Optimize trivial phis.
+  for (auto &MP : InsertedPHIs) {
+    MemoryPhi *MPhi = dyn_cast_or_null<MemoryPhi>(MP);
----------------
Why are we looping over all of the `InsertedPhis`? Shouldn't this just be the subset of them that we added as a part of the IDF stuff above? (Any Phis we add as a part of `getPreviousDef` should already be minimal, AIUI)


================
Comment at: lib/Analysis/MemorySSAUpdater.cpp:342
+  for (auto &MP : InsertedPHIs) {
+    MemoryPhi *MPhi = dyn_cast_or_null<MemoryPhi>(MP);
+    if (MPhi) {
----------------
nit: should this be cast_or_null? `WeakVH` doesn't track across RAUWs IIRC

Please also bring this into the `if (...)`


================
Comment at: test/Analysis/MemorySSA/pr40749.ll:1
+; RUN: opt -licm -enable-mssa-loop-dependency -verify-memoryssa -S < %s | FileCheck %s
+
----------------
`; REQUIRES: asserts`?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58652