[PATCH] D138636: [LSR] Hoist IVInc to loop header if its all uses are in the loop header

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5653
 
+// Check the instruction's all user is in the basicblock
+static bool isUserAllWithinBB(Value *V, BasicBlock *BB) {
----------------
nit: Check if all users of \p V are in \p BB?

name should use `are` instead of `is` and `Users`, as this checks all users.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5655
+static bool isUserAllWithinBB(Value *V, BasicBlock *BB) {
+  for (User *U : V->users()) {
+    Instruction *UI = cast<Instruction>(U);
----------------
nit: use any_of?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5663
+
+// Trying to hoist the IVInc to loop header if all IVInc user is in
+// the loop header. It will help backend to generate post index load/store
----------------
...all IVInc users are ...


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5684
+  Instruction *I = Fixup.UserInst;
+  if ((isa<LoadInst>(I) &&
+       TTI.isIndexedLoadLegal(TTI.MIM_PostInc, I->getType())) ||
----------------
can just `return (isa<LoadInst>(I) &&.....`


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/AArch64/pr53625.ll:101
+for.body:                                         ; preds = %for.body.preheader, %for.cond
+  %indvars.iv = phi i64 [ 0, %for.body.preheader ], [ %indvars.iv.next, %for.cond ]
+  %indvars.iv.a = phi i64 [ 1, %for.body.preheader ], [ %indvars.iv.next.a, %for.cond ]
----------------
nit: drop redundant `indvars.` prefix to make IR more concise.


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

https://reviews.llvm.org/D138636