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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer marked an inline comment as done.
SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:241
+  if (!Latch || !Latch->hasOneUse() ||
+      Latch->getSignedPredicate() != ICmpInst::ICMP_SGT)
+    return nullptr;
----------------
samparker wrote:
> So, is this the canonical form? We won't see ICMP_NE? 
More canonical is ICMP_EQ, which is what you'll get when you e.g. have `while (N--)` instead of the case `while (N-- > 0)` supported here. But at the same time time, I think we can have ICMP_NE too.
I considered supporting ICMP_SGT first as the minimal viable product, also to check how you reviewers and testing appreciate this change and approach, then follow-up to support some more predicates :-) 


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:277
+  // Create new increment, compare and branch instructions.
+  Value *NewIndOp = IRB.CreateAdd(
+      IndVar,
----------------
samparker wrote:
> Have we checked that the original indvar only has one use?
Don't think so, thanks, will do.


================
Comment at: llvm/lib/Analysis/LoopInfo.cpp:281
+  IndOp->replaceAllUsesWith(NewIndOp);
+  IRB.CreateCondBr(IRB.CreateICmpEQ(NewIndOp, StartValue), ExitBlock,
+                   IndVar->getParent());
----------------
samparker wrote:
> I guess you could just create a cmp and replace the users, so we won't have to delete and create another branch.
Because of the EQ predicate, I have the TRUE and FALSE block operands swapped compared to the original 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),
----------------
samparker wrote:
> I think just using setIncoming will be fine.
That's what I wanted, but if I haven't overlooked anything, that requires an index, which means I need to do find the index first, then do setIncoming, and then I might as well remove it and add the updated one. 


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

https://reviews.llvm.org/D76838