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

[chenglin.bi via Phabricator via llvm-commits]

bcl5980 added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5674
+
+  if (IVIncInsertPos->getParent() != L->getLoopLatch())
+    return false;
----------------
eopXD wrote:
> eopXD wrote:
> > The condition here makes the previous early-return condition redundant.
> > 
> Possibly a negative test case for this?
I'm not sure how to get the assertion. Why not in latch means it is in the header?


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5677
+
+  Instruction *User = dyn_cast<Instruction>(Fixup.OperandValToReplace);
+  if (!User || !AllUseInBB(User, LHeader))
----------------
eopXD wrote:
> For the sake of my own knowledge, may I ask why does this necessary has to be an `Instruction`?
I can change to Value here, but I feel strange if it is a constant.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5681
+
+  Instruction *I = Fixup.UserInst;
+  if ((isa<LoadInst>(I) &&
----------------
eopXD wrote:
> This looks familiar with `mayUsePostIncMode`
It looks `mayUsePostIncMode` is not what I want. It only checks the instruction is legal but haven't check the instruction opcode.


================
Comment at: llvm/test/Transforms/LoopStrengthReduce/ivincs-hoist.ll:4
 
+target triple = "aarch64-unknown-linux-gnu"
 target datalayout = "e-m:w-p:64:64-i32:32-i64:64-i128:128-n32:64-S128"
----------------
eopXD wrote:
> As a generic test case here (not under a specific target), is there other ways to show the effect of this patch without adding this target triple?
The headache thing is we must have a target to enable the post-index load/store to enable the optimization. I don't know how to show the code change without target triple info.


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

https://reviews.llvm.org/D138636