[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.

Minor comments on style and redundant code, the change looks reasonable to me and resolves the problem.



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5652
 
+static bool AllUseInBB(Instruction *I, BasicBlock *BB) {
+  for (User *U : I->users()) {
----------------
Personally I think `isAllUsedInBB` (or `isUserAllWithinBB`) is a more suitable name that follows general naming rules across the codebase.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5652
 
+static bool AllUseInBB(Instruction *I, BasicBlock *BB) {
+  for (User *U : I->users()) {
----------------
eopXD wrote:
> Personally I think `isAllUsedInBB` (or `isUserAllWithinBB`) is a more suitable name that follows general naming rules across the codebase.
Also, maybe a comment for the function for clarity.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5664
+// when the latch block is different from loop header block.
+static bool CanHoistIVInc(const TargetTransformInfo &TTI, const LSRFixup &Fixup,
+                          const LSRUse &LU, Instruction *IVIncInsertPos,
----------------
`CanHoistIVInc` -> `canHoistIVInc`.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5674
+
+  if (IVIncInsertPos->getParent() != L->getLoopLatch())
+    return false;
----------------
The condition here makes the previous early-return condition redundant.



================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5674
+
+  if (IVIncInsertPos->getParent() != L->getLoopLatch())
+    return false;
----------------
eopXD wrote:
> The condition here makes the previous early-return condition redundant.
> 
Possibly a negative test case for this?


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


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5681
+
+  Instruction *I = Fixup.UserInst;
+  if ((isa<LoadInst>(I) &&
----------------
This looks familiar with `mayUsePostIncMode`


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:5708
+    for (const LSRFixup &Fixup : LU.Fixups) {
+      Instruction *InsertPos = CanHoistIVInc(TTI, Fixup, LU, IVIncInsertPos, L)
+                                   ? L->getHeader()->getTerminator()
----------------
No need to change the nested loop here, can do

```
/* ... */ = canHoistIVInc(TTI, Fixup, Uses[LUIdx], /* ... */);
```


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

https://reviews.llvm.org/D138636