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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:46
+/// ordering.
+
+static BasicBlock *usersDominator(Instruction *I, DominatorTree &DT,
----------------
delete blank line.

`// If I exists and is deeper ...`

What does `deeper` mean? In the loop body, `if (AA.getModRefInfo(MI, ML) != ModRefInfo::NoModRef && !MI->isLifetimeStartOrEnd() && MI != I) {` does something unclear but does not have a comment.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:53
+    ML = MemoryLocation::getForDest(MI);
+  else if (auto *MI = dyn_cast<MemIntrinsic>(I))
+    ML = MemoryLocation::getForDest(MI);
----------------
For auto-init instructions, MemIntrinsic is sufficient and AnyMemTransferInst is unneeded.


================
Comment at: llvm/lib/Transforms/Utils/MoveAutoInit.cpp:104
+
+    // FIXME: this guards against generalization of this pass to any statement.
+    // The remaining logic should be pretty similar.
----------------
The pass name is MoveAutoInit. Handling just auto-init instructions is what the pass is designed for, so I don't think not generalizing it deserves a FIXME. You can drop FIXME.


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

https://reviews.llvm.org/D137707