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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:23
+#include "llvm/IR/Instructions.h"
+#include "llvm/IR/IntrinsicInst.h"
+#include "llvm/Support/CommandLine.h"
----------------
are you still using this header? I didn't see any references to any intrinsics. Can you recheck these? DebugInfo.h I would think is also unused.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:44
+
+BasicBlock *usersDominator(Instruction *I, DominatorTree &DT, MemorySSA &MSSA) {
+  BasicBlock *CurrentDominator = nullptr;
----------------
Mind adding a comment for this function?


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:51-55
+  SmallVector<MemoryAccess *> WorkList;
+  for (User *U : IMA->users()) {
+    auto *MA = cast<MemoryAccess>(U);
+    WorkList.push_back(MA);
+  }
----------------
You might be able to eliminate this for loop by using llvm::make_pointer_range (llvm/include/llvm/ADT/iterator.h) to initialize the Worklist.  You'd probably need to move the cast to the below loop though.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:63
+    if (auto *M = dyn_cast<MemoryUseOrDef>(MA)) {
+      if (auto *MI = M->getMemoryInst()) {
+        if (MI->isLifetimeStartOrEnd() || MI == I)
----------------
Instruction


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:72-73
+    } else if (auto *M = dyn_cast<MemoryPhi>(MA)) {
+      for (unsigned i = 0, n = M->getNumIncomingValues(); i != n; ++i)
+        WorkList.push_back(M->getIncomingValue(i));
+    }
----------------
Are you able to use a range-for here with `MemoryPhi::incoming_values()`?


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:103-104
+    SmallPtrSet<BasicBlock *, 8> TransitiveSuccessors;
+    SmallVector<BasicBlock *> WorkList(succ_begin(UsersDominator),
+                                       succ_end(UsersDominator));
+    bool HasCycle = false;
----------------
I think you can just pass `successors(UsersDominator)`?


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:133-134
+      for (BasicBlock *Pred : predecessors(UsersDominatorHead)) {
+        // If one of the predecessor also transitively is a successor, that's
+        // not the path we don't consider that path.
+        if (TransitiveSuccessors.count(Pred))
----------------
This sentence could be reworded. Looks like it was partially updated at some point?


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

https://reviews.llvm.org/D137707