[PATCH] New pass: inductive range check elimination

[Philip Reames]

As Hal and I said previously, LGTM so that you can continue to work on this incrementally in tree.   Each change should be reviewed incrementally, and when you ready to insert this in the standard pass order, we'll need to do another holistic review.


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:16
@@ +15,3 @@
+//   for (i = 0; i < n; i++) {
+//     if (0 <= i && i < len) {
+//       do_something();
----------------
I think your example would be clearer with the full generality of three loops.  No strong preference though.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:697
@@ +696,3 @@
+void LoopConstrainer::cloneLoop(LoopConstrainer::ClonedLoop &Result,
+                                const char *Tag) const {
+  for (BasicBlock *BB : OriginalLoop->getBlocks()) {
----------------
Is there a utility function somewhere you could use for this?  Or can you extract one?  

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:749
@@ +748,3 @@
+LoopConstrainer::rewriteIterationRange(const LoopStructure &LS,
+                                       BasicBlock *Preheader, Value *ExitLoopAt,
+                                       BasicBlock *ContinuationBlock) const {
----------------
FYI, as a name rewriteIterationRange doesn't tell me that much.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:852
@@ +851,3 @@
+
+    PHINode *PN = cast<PHINode>(&I);
+    for (unsigned i = 0, e = PN->getNumIncomingValues(); i < e; ++i)
----------------
The fact you have these functions at all seems suspicious.  I suspect you could get quite far by using the utilities in BasicBlockUtils.  

e.g.
NewPreheader = SplitEdge(OriginalPreheader, OriginalHeader)


================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:895
@@ +894,3 @@
+
+    MainLoopPreheader =
+        createPreheader(MainLoopStructure, Preheader, "mainloop");
----------------
It really feels like you're mixing the flow here.  I don't have a concrete suggestion, but you might try inserting a legal loop in its entirety, then rewriting it to be restricted to the proper range.  Alternatively, rewrite the free standing loop in it's entirety, then insert.  Mixing things is confusing.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:920
@@ +919,3 @@
+  // verify this after running a loop pass.
+  if (Loop *ParentLoop = OriginalLoop->getParentLoop()) {
+    auto &LoopInfoBase = OriginalLoopInfo->getBase();
----------------
Helper function?

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:934
@@ +933,3 @@
+
+    for (BasicBlock *BB : PreLoop.Blocks)
+      ParentLoop->addBasicBlockToLoop(BB, LoopInfoBase);
----------------
Is this valid if the loop is not created?

p.s. Your LoopClone *really* looks like LoopInfo.  

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1055
@@ +1054,3 @@
+
+  LoopConstrainer LC(L, &getAnalysis<LoopInfo>(), SE, SafeIterRange.getValue());
+  bool Changed = LC.run();
----------------
I'm really not a fan of MethodObject pattern.  It's better than what you had, but please don't keep this long term.  :)

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1069
@@ +1068,3 @@
+
+#ifndef NDEBUG
+    // In the future we'd like to assert this in debug mode and directly do the
----------------
As we talked about offline, I really think that unconditionally rewriting the branches based on inferred knowledge is the right approach.  You can emit a runtime check for diagnostics/debugging if desired, but the core behaviour of the pass should not change between debug and non-debug builds.  You also don't want to tightly couple this pass with the effectiveness of other parts of the optimizer.

This is the one *must fix* I have.  Even here, a follow up change is fine.

================
Comment at: test/Transforms/InductiveRangeCheckElimination/multiple-access-no-preloop.ll:3
@@ +2,3 @@
+
+define void @multiple_access_no_preloop(
+    i32* %arr_a, i32* %a_len_ptr, i32* %arr_b, i32* %b_len_ptr, i32 %n) {
----------------
Mild personal preference: I'd name the directory IRCE to avoid something so long.  

================
Comment at: test/Transforms/InductiveRangeCheckElimination/multiple-access-no-preloop.ll:12
@@ +11,3 @@
+
+; CHECK-LABEL: loop.preheader:
+; CHECK: [[smaller_len_cmp:[^ ]+]] = icmp slt i32 %len.a, %len.b
----------------
Intermixing checks with the source IR is somewhat confusing with this radical a transform.  

================
Comment at: test/Transforms/InductiveRangeCheckElimination/single-access-no-preloop.ll:3
@@ +2,3 @@
+
+define void @single_access_no_preloop(i32 *%arr, i32 *%a_len_ptr, i32 %n) {
+ entry:
----------------
It looks like all of these tests could be in a single file?

p.s. You *need* more tests.  While I'm okay with this landing without them so that you can work incrementally, every follow up change will need a motivating test.  You current coverage is minimal at best.  Ideas: loops which don't match, conditions you can't handle, a starter loop which isn't in loop-simplify form...

http://reviews.llvm.org/D6693

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/