[PATCH] D76838: [LV][LoopInfo] Transform counting-down loops to counting-up loop

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:241
+  if (!Latch || !Latch->hasOneUse() ||
+      Latch->getSignedPredicate() != ICmpInst::ICMP_SGT)
+    return nullptr;
----------------
So, is this the canonical form? We won't see ICMP_NE? 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:245
+  User *Br = *Latch->user_begin();
+  if (!dyn_cast<BranchInst>(Br))
+    return nullptr;
----------------
isa<> or use dyn_cast in the line above.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:253
+  ConstantInt *LatchRHS = dyn_cast<ConstantInt>(Latch->getOperand(1));
+  if (Latch->getOperand(0) != IndVar || !LatchRHS || !LatchRHS->isOne())
+    return nullptr;
----------------
Same query about ICMP_SGT 1 and ICMP_NE 0.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:277
+  // Create new increment, compare and branch instructions.
+  Value *NewIndOp = IRB.CreateAdd(
+      IndVar,
----------------
Have we checked that the original indvar only has one use?


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:281
+  IndOp->replaceAllUsesWith(NewIndOp);
+  IRB.CreateCondBr(IRB.CreateICmpEQ(NewIndOp, StartValue), ExitBlock,
+                   IndVar->getParent());
----------------
I guess you could just create a cmp and replace the users, so we won't have to delete and create another branch.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:290
+  // Modify the Phi node with the new start value, i.e. 1.
+  IndVar->removeIncomingValue(Preheader);
+  IndVar->addIncoming(llvm::ConstantInt::get(IndOp->getType(), 1),
----------------
I think just using setIncoming will be fine.


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

https://reviews.llvm.org/D76838