[PATCH] D22630: Loop rotation

[Michael Zolotukhin via Phabricator via llvm-commits]

mzolotukhin requested changes to this revision.
mzolotukhin added a comment.
This revision now requires changes to proceed.

Please fix the tests as requested.

We need to cover multiple scenarios by a minimal set of tests. There is no need in combining scenarios unless it triggers some corner case (in which case we should explicitly state which corner case we're testing). For instance, using a custom type `%fp` is completely irrelevant from what loop-rotation is doing, so it should be removed from the test. Many tests contain unreachable blocks (I realize you get them from bugpoint) - what's the purpose of keeping them in the test? Why can't we remove them? And so forth...

Michael



================
Comment at: llvm/lib/Transforms/Scalar/LoopRotation.cpp:604
+        DEBUG(dbgs() << "\nChanging IDom of " << *ExitBB << "to" << *NewIDom);
+        I = DTBB->begin();
+      } else
----------------
hiraditya wrote:
> mzolotukhin wrote:
> > Can we avoid quadratic complexity here?
> Actually this while loop is bounded by the number of exiting basicblocks from the loop which were cloned.
Can you elaborate? I don't see any exits from the loop, but I see how we start from the beginning.


================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate.ll:149-153
+bb10:
+  switch i32 %arg3, label %bb11 [
+    i32 46, label %bb19
+    i32 32, label %bb19
+  ]
----------------
hiraditya wrote:
> mzolotukhin wrote:
> > Why do we need two switches in this test?
> This switch statement is a loop preheader as it is branching to the loop header. So I think, it would exercise if the dominators are updated properly.
Loop simplify splits the edge to the loop anyway. Thus, this test, if checking anything, checks only loop simplify. For testing loop-rotation the first switch is absolutely unnecessary.


================
Comment at: llvm/test/Transforms/LoopRotate/loop-rotate.ll:11
+
+%fp = type { i64 }
+
----------------
Why can't we use `i64` instead of `%fp`?


https://reviews.llvm.org/D22630