[PATCH] New pass: inductive range check elimination

[Sanjoy Das]

Comments replied to inline.  I'll address some of the easier ones before checking in.


================
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()) {
----------------
reames wrote:
> Is there a utility function somewhere you could use for this?  Or can you extract one?  
> Is there a utility function somewhere you could use for this?

I could not find any.

> Or can you extract one?

I think that is good idea as long as I can find another user.  I plan to loop at both `LoopUnroll` and `LoopRotate` for this in the future.

================
Comment at: lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:749
@@ +748,3 @@
+LoopConstrainer::rewriteIterationRange(const LoopStructure &LS,
+                                       BasicBlock *Preheader, Value *ExitLoopAt,
+                                       BasicBlock *ContinuationBlock) const {
----------------
reames wrote:
> FYI, as a name rewriteIterationRange doesn't tell me that much.
I'll change it to something more descriptive before checking in.

================
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)
----------------
reames wrote:
> 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)
> 
> NewPreheader = SplitEdge(OriginalPreheader, OriginalHeader)

That's a great idea, I'll do that before checking in.

================
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();
----------------
reames wrote:
> Helper function?
Will do before checkin.

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

Yes -- `PreLoop.Blocks` is empty in that case.

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

The crucial difference is that there is a bunch of stuff `LoopInfo` //computes// that is statically cached in a `LoopClone`.  This lets me use `LoopClone` while I'm transforming the IR.  For instance, I don't think it is okay to call `LoopInfo::getLoopLatch` if the loop is not well-formed.  I'll document this difference in a comment.

================
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
----------------
reames wrote:
> 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.
I agree -- the code as of now is basically to help debugging.

================
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) {
----------------
reames wrote:
> Mild personal preference: I'd name the directory IRCE to avoid something so long.  
Good idea, will do that before checkin.

================
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:
----------------
reames wrote:
> 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...
> It looks like all of these tests could be in a single file?

These files are really categories of tests that I will fill in.

> p.s. You *need* more tests.

Completely agree.  In fact, I plan to add at least a few more tests before checking in.

http://reviews.llvm.org/D6693

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