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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers requested changes to this revision.
nickdesaulniers added a comment.
This revision now requires changes to proceed.

No comment on the approach. Minor drive by comments on style.  But I'd like to see IR tests added for this pass.  I haven't run it yet on the Linux kernel but can do so to provide measurements.



================
Comment at: llvm/include/llvm/InitializePasses.h:201
 void initializeLCSSAVerificationPassPass(PassRegistry&);
-void initializeLCSSAWrapperPassPass(PassRegistry&);
 void initializeLazyBlockFrequencyInfoPassPass(PassRegistry&);
----------------
drop this hunk


================
Comment at: llvm/include/llvm/LinkAllPasses.h:116
       (void) llvm::createJMCInstrumenterPass();
-      (void) llvm::createLCSSAPass();
       (void) llvm::createLegacyDivergenceAnalysisPass();
----------------
drop this hunk


================
Comment at: llvm/include/llvm/LinkAllPasses.h:139
       (void) llvm::createLowerSwitchPass();
+      (void)llvm::createMoveAutoInitPass();
       (void) llvm::createNaryReassociatePass();
----------------
add space before cast


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:49
+    MemOp = SI->getPointerOperand();
+  }
+
----------------
delete


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:72
+  for (User *U : V->users()) {
+    if (Instruction *UI = dyn_cast<Instruction>(U)) {
+      if (UI->isLifetimeStartOrEnd() || UI == I)
----------------
auto


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:80
+            DT.findNearestCommonDominator(CurrentDominator, UI->getParent());
+      }
+    }
----------------
delete.

See comment below about ternaries, which also applies here.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:125
+        HasCycle = true;
+      }
+      for (BasicBlock *Successor : successors(CurrBB)) {
----------------
delete


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:129
+          continue;
+        }
+        WorkList.push_back(Successor);
----------------
delete


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:150-154
+        if (!DominatingPredecessor)
+          DominatingPredecessor = Pred;
+        else
+          DominatingPredecessor =
+              DT.findNearestCommonDominator(DominatingPredecessor, Pred);
----------------
Please use

```
if (x)
  y()
else
  z()
```
rather than
```
if (!x)
  z()
else
  y()
```
Additionally, since we're assigning to the same variable, consider using a ternary statement.

```
DominatingPredecessor = (DominatingPredecessor ? DT.find... : Pred)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137707