[PATCH] D137707: Move "auto-init" instructions to the dominator of their users

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:58
+  MemoryUseOrDef &IMA = *MSSA.getMemoryAccess(I);
+  auto &AA = MSSA.getAA();
+
----------------



================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:68
+    if (!Visited.insert(MA).second)
+      continue;
+
----------------
```
if (Visited.size() > 128)
  return nullptr;
```
for some value of 128 :)


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:76
+      // LifetimeEnd is a valid user, but we do not want it in the user
+      // dominator.
+      if (AA.getModRefInfo(MI, ML) != ModRefInfo::NoModRef &&
----------------
Doesn't this mean we may potentially move the initialization past the lifetime.end?


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:137
+                 UsersDominatorHead->getUniquePredecessor())
+        UsersDominatorHead = UniquePredecessor;
+
----------------
This will miss cases where the dominator is not the cycle header, right? Possible improvement for the future.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:171
+      for (BasicBlock *Pred : predecessors(UsersDominator))
+        UsersDominator = DT.findNearestCommonDominator(UsersDominator, Pred);
+      UsersDominatorInsertionPt = &*UsersDominator->getFirstInsertionPt();
----------------
The common dominator of all the predecessors is probably just the idom?


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:193
+  if (VerifyMemorySSA)
+    MSSA.verifyMemorySSA();
+
----------------
Could you please also add `-verify-memoryssa` to the tests?


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

https://reviews.llvm.org/D137707