[PATCH] D105802: [LoopFlatten] Fix missed LoopFlatten opportunity

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:169
     Limit = Compare->getOperand(0);
+  } else {
+    // The compare may have been altered by another transformation
----------------
Make this an `else if`?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:178
+      Increment = dyn_cast<BinaryOperator>(LatchValue);
+      ConstantInt *RHS = cast<ConstantInt>(Compare->getOperand(1));
+      ConstantInt *One = ConstantInt::get(RHS->getType(), 1, true);
----------------
I am wondering what happens if the RHS is not a constant, so instead of this:

  %cmp = icmp ult i32 %j, 18

we have something like:

    %cmp = icmp ult i32 %j, %N

Do we reject that earlier, and/or do we not have these test cases yet?


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:181
+      Limit = ConstantInt::get(Compare->getContext(),
+                               RHS->getValue() + One->getValue());
+    }
----------------
And I was also wondering if we need to worry about RHS being INT_MAX, in which case the + 1 will overflow.


================
Comment at: llvm/lib/Transforms/Scalar/LoopFlatten.cpp:395
                             m_Value(MatchedItCount)));
 
     if ((IsAdd || IsAddTrunc) && MatchedItCount == InnerLimit) {
----------------
Perhaps here we can have:

   // The use is in the compare which is also the condition of the inner
   // branch. In this case the compare has been altered by another
   // transformation (e.g icmp ult %inc, limit -> icmp ult %j, limit-1).
   // Ignore this use as the compare gets removed later anyway.
   if (U == FI.InnerBranch->getCondition())
      continue;

To make things a bit simpler.


================
Comment at: llvm/test/Transforms/LoopFlatten/loop-flatten-negative.ll:348
+
+; test_10: If the step is not 1, the loop shouldn't be flattened.
+define i32 @test_10(i32* nocapture %A) {
----------------
You need to `CHECK:` something here.


================
Comment at: llvm/test/Transforms/LoopFlatten/loop-flatten-negative.ll:379
+; isn't the condition of the inner branch.
+define i32 @test_11(i32* nocapture %A) {
+entry:
----------------
Same here.


================
Comment at: llvm/test/Transforms/LoopFlatten/loop-flatten.ll:589
 
+; CHECK-LABEL: test9
+; When the inner loop limit is a defined integer (e.g. 20) and the step
----------------
It's more common to put the CHECK-LABEL just before

   ; CHECK: entry:


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105802