[PATCH] D132443: [LSR] Fold terminal condition to other IV when possible

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

eopXD added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:189
 
+static cl::opt<bool> AllowTerminalConditionFoldingAfterLSR(
+    "lsr-term-fold", cl::Hidden, cl::init(false),
----------------
craig.topper wrote:
> Terminal -> Terminating? The word terminal does not appear in LSR prior to this patch.
Replaced all use of "terminal condition" to "terminating condition". 


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:191-193
+    cl::desc("If this option is set to true, LSR will attempt to see if the "
+             "primary IV that is only used to by the terminal condition can be "
+             "replaced by other IV."));
----------------
hiraditya wrote:
>     cl::desc("Attempt to replace primary IV with other IV."));
> 
Thank you for the simplification. Was trying to explain in detail but this is much better.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6581
 
+static std::pair<bool, std::pair<PHINode *, PHINode *>>
+canFoldTermCondOfLoop(Loop *L, ScalarEvolution &SE, DominatorTree &DT,
----------------
craig.topper wrote:
> std::tuple?
Thank you for the tip, done.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6613
+    return CantFold;
+  if (!cast<ICmpInst>(TermCond)->isEquality())
+    return CantFold;
----------------
craig.topper wrote:
> Combine this if with the previous if
> 
> ```
> if (!isa<ICmpInst>(TermCond) || !cast<ICmpInst>(TermCond)->isEquality())
> ```
Done.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6669
+
+    for (unsigned I = 0; I < PN.getNumIncomingValues(); ++I) {
+      if (PN.getIncomingBlock(I) == LoopPreheader)
----------------
craig.topper wrote:
> Can this use PN.getIncomingValueForBlock or getBasicBlockIndex?
This simplifies the code very much. Thank you for the tip.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6699
+      continue;
+    if (!AddRec->isAffine())
+      continue;
----------------
craig.topper wrote:
> Combine this with the previous if
Done. Thank you.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6798
+      for (Use &U : ToHelpFold->operands()) {
+        if (ToHelpFold->getIncomingBlock(U) == LoopPreheader)
+          StartValue = cast<Value>(&U);
----------------
craig.topper wrote:
> Is this loop just `ToHelpFold->getIncomingValueForBlock(LoopPreheader)`?
Yes, thank you.


================
Comment at: llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp:6799
+        if (ToHelpFold->getIncomingBlock(U) == LoopPreheader)
+          StartValue = cast<Value>(&U);
+        else
----------------
craig.topper wrote:
> Do you really need a cast here? If you do, I'd use U.get() instead.
The problem is elegantly solved by using `PHINode:: getIncomingValueForBlock` you've mentioned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132443