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

[Yueh-Ting (eop) Chen via Phabricator via llvm-commits]

eopXD added a comment.

Sorry for the delay. Minor comments left and I think we are good to land this.



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5674
+
+  if (IVIncInsertPos->getParent() != L->getLoopLatch())
+    return false;
----------------
bcl5980 wrote:
> 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?
Use LLVM_DEBUG and have the following at the first line of the test case (.ll file)

```
; REQUIRES: asserts
```


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5681
+
+  Instruction *I = Fixup.UserInst;
+  if ((isa<LoadInst>(I) &&
----------------
bcl5980 wrote:
> 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.
```
if ((isa<LoadInst>(I) || isa<StoreInst>(I)) && mayUsePostIncMode(...)
```

Just trying my best to reuse existing code :)


================
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"
----------------
bcl5980 wrote:
> 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.
Then I guess the target triple here should not be added. You already have the test case under AaArch64 for this.


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

https://reviews.llvm.org/D138636