[PATCH] Loop Versioning for LICM

[hfinkel at anl.gov]

First, thanks for working on this. I apologize for the delay in looking at it.

Some high-level comments first...

Like the vectorizer, you'll need some metadata that you can add to the loop so that, when this pass is run multiple times over the same code, it does not recursively multi-version the same loop.

This pass needs some nice descriptive text near the top. The summary you wrote for this review would make a good start (added as a comment near the top of the file).


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:253
@@ +252,3 @@
+    MPM.add(createLoopVersioningPass());        // Do Loop Versioning
+    MPM.add(createLICMPass());                  // Hoist loop invariants
+  }
----------------
Why are you running LICM here? I thought that the versioning pass had an internal LICM? Or is this to hoist the checks?


================
Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:466
@@ +465,3 @@
+  // 3) Check store instruction execution guarantee.
+  // 4) Loop body should have a invariant store.
+  // 5) Loop body should not have any may throw instruction.
----------------
What is special about having an invariant store?

If we're interested in hoisting loop invariant loads that we cannot prove are loop invariant, we can still have a problem if a loop-varying store aliasing with the load in any iteration, no?


================
Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:476
@@ +475,3 @@
+      CallInst *CI = dyn_cast<CallInst>(it);
+      if (CI && !isa<DbgInfoIntrinsic>(CI)) {
+        DEBUG(dbgs() << "    loop body has a call instruction\n");
----------------
This is a bit overkill. You should certainly ignore readnone calls. Why not reuse the logic here currently used by the vectorizer to decide which calls are safe?


================
Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:518
@@ +517,3 @@
+    } // Next instr.
+  }   // Next block.
+  LAI = &LAA->getInfo(CurLoop, Strides);
----------------
If you feel the need to add these kinds of comments on the closing curly brace, please split out a helper function instead.


================
Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:811
@@ +810,3 @@
+    // Create alias scope domain.
+    MDBuilder MDB(I->getContext());
+    MDNode *NewDomain = MDB.createAnonymousAliasScopeDomain("LVDomain");
----------------
This metadata-adding code should be a separate function.


================
Comment at: lib/Transforms/Scalar/LoopVersioning.cpp:831
@@ +830,3 @@
+        NoAliases.push_back(NewScope);
+        Scopes.push_back(NewScope);
+        // Set no-alias for current instruction.
----------------
I don't think this is right because you'll mark as noalias access to the same underlying object that might, in fact, alias. We'll need to change this slightly, and this will become easier, once we have the upcoming llvm.noalias intrinsic.

http://reviews.llvm.org/D9151

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