[PATCH] D44766: Extend peeling to help invariant motion

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
Comment at: lib/Analysis/LoopInfo.cpp:439
+  if (UniqueExitBlocks.size() == 1)
+    return UniqueExitBlocks[0];
+  BasicBlock *ReachableExit = nullptr;
----------------
This will break semantics of this method in case if this block is exit/unreachable/etc.


================
Comment at: lib/Analysis/LoopInfo.cpp:452
+    // If found second reachable exit return nullptr.
+    if (ReachableExit) {
+      return nullptr;
----------------
Braces not needed.


================
Comment at: lib/Analysis/LoopInfo.cpp:464
+  if (ExitingBlocks.size() == 1)
+    return ExitingBlocks[0];
+  BasicBlock *ExitingToReachable = nullptr;
----------------
Same, you should also check this block on being exit/unreachable.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:240
+    L->getUniqueExitBlocks(ExitBlocks);
+    bool SafeToSpeculate;
+    for (BasicBlock *BB : L->blocks()) {
----------------
Uninitialized variable will cause UB on line 250.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:242
+    for (BasicBlock *BB : L->blocks()) {
+      for (BasicBlock *EB : ExitBlocks) {
+        if (!DT.dominates(BB, EB)) {
----------------
Braces not needed.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:254
+      for (Instruction &I : *BB) {
+        if (isa<StoreInst>(&I) || isa<CallInst>(&I))
+          continue;
----------------
I think `isGuaranteedToTransferExecutionToSuccessor` is what you actually need here.


================
Comment at: lib/Transforms/Utils/LoopUnrollPeel.cpp:637
+        continue;
+      for (BasicBlock::iterator I = EB->begin(); isa<PHINode>(I); ++I) {
+        PHINode *PHI = cast<PHINode>(I);
----------------
Could be `const PHINode &PHI : EB->phis()`


Repository:
  rL LLVM

https://reviews.llvm.org/D44766