[PATCH] D22630: Loop rotation

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Aditya,

I apologize, I think I posted a wrong example (as you mentioned, the current patch has no problems with it).
Here is a slightly modified version, which causes some problems though:

  declare void @f()
  declare void @use(i32)
  
  define void @test2(i32 %v) {
  entry:
    %add = add nsw i32 %v, 1
    br label %for_header
  
  for_header:
    %iv = phi i32 [ 0, %entry ], [ %inc, %for_crit ]
    %cond = icmp eq i32 %iv, 1
    br i1 %cond, label %for_if, label %for_exit
  
  for_if:
    call void @f()
    br label %for_exit
  
  for_exit:
    %inc = add nsw i32 %iv, 1
    %cmp = icmp slt i32 %iv, 1
    br i1 %cmp, label %for_crit, label %for_end
  
  for_crit:
    call void @use(i32 %iv)
    br label %for_header
  
  for_end:
    ret void
  }

The current patch bails out on the condition you've already discussed with Eli: ` if (!L->isLoopExiting(OrigHeader))`. Provided we properly compute the cost of the rotated loop, I see no reason not to rotate this loop.

Michael


================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:105
@@ -59,2 +104,3 @@
   DominatorTree *DT;
-  ScalarEvolution *SE;
+  Function *F;
+  Loop *L;
----------------
I don't see much value in having `F` as a member of `LoopRotate`. It's not used very often, and where it's used it's easy to just get `Header->getParent()`.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:106
@@ -61,1 +105,3 @@
+  Function *F;
+  Loop *L;
 
----------------
I think adding a member `L` to `class LoopRotate` might go in a separate NFC patch (but it's ok to keep it here too - whatever you prefer).

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:334
@@ +333,3 @@
+    }
+  assert(BeforeLoop);
+
----------------
A message is missing.

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:396
@@ -583,5 +395,3 @@
 
-  // Remove Latch from the CFG so that LastExit becomes the new Latch.
-  BI->setSuccessor(FallThruPath, Header);
-  Latch->replaceSuccessorsPhiUsesWith(LastExit);
-  Jmp->eraseFromParent();
+  assert(L->isLCSSAForm(*DT) && "Loop is not in LCSSA form.");
+  LI->verify();
----------------
Can we also verify DT and LoopSimplify (if it's valid to assume that the loop is in simplified form after rotation)?

================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:397-398
@@ +396,4 @@
+  assert(L->isLCSSAForm(*DT) && "Loop is not in LCSSA form.");
+  LI->verify();
+  verifyFunction(*F);
+  DEBUG(dbgs() << "LoopRotation: into "; L->dumpVerbose());
----------------
Should it be under `#ifndef NDEBUG`?

================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate-6.ll:13-14
@@ +12,4 @@
+
+; <label>:5:                                      ; preds = %4
+  unreachable
+
----------------
This test would benefit from `-instnamer` and manual cleanups.

================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate-9.ll:18-25
@@ +17,10 @@
+
+if.then39:                                        ; preds = %if.end
+  unreachable
+
+if.end48:                                         ; preds = %if.end
+  br i1 undef, label %if.else61, label %if.then59
+
+if.then59:                                        ; preds = %if.end48
+  unreachable
+
----------------
I'm pretty sure this test can be cleaned up further (manually). I also suggest moving all 'crasher' tests to one file.


https://reviews.llvm.org/D22630