[PATCH] D9151: Loop Versioning for LICM

[Hal Finkel via llvm-commits]

hfinkel added a comment.

I apologize for taking so long to get back to this...

Do the runtme checks inserted cover only the interactions between the invariant access and the non-invariant accesses, or do they also perform range overlap checks on the non-invariant accesses?


================
Comment at: docs/LangRef.rst:4386
@@ +4385,3 @@
+This metadata indicates that the loop is already versioned by loop versioning
+for loop invariant code motion(LICM). This metadata is applicable on both 
+original loop and clone loop. It helps suggesting loop-versioning-licm pass that
----------------
Add a space just before "(LICM)".

================
Comment at: docs/LangRef.rst:4387
@@ +4386,3 @@
+for loop invariant code motion(LICM). This metadata is applicable on both 
+original loop and clone loop. It helps suggesting loop-versioning-licm pass that
+the loop should not be re-versioned. The metadata has a single operand which is 
----------------
clone loop -> all other loop versions

================
Comment at: docs/LangRef.rst:4387
@@ +4386,3 @@
+for loop invariant code motion(LICM). This metadata is applicable on both 
+original loop and clone loop. It helps suggesting loop-versioning-licm pass that
+the loop should not be re-versioned. The metadata has a single operand which is 
----------------
hfinkel wrote:
> clone loop -> all other loop versions
original loop -> the original loop

================
Comment at: docs/LangRef.rst:4388
@@ +4387,3 @@
+original loop and clone loop. It helps suggesting loop-versioning-licm pass that
+the loop should not be re-versioned. The metadata has a single operand which is 
+the string ``llvm.loop.licm_versioning``.
----------------
It helps suggesting loop-versioning-licm pass that the loop should not be re-versioned. -> The loop-versioning-licm pass will not create additional loop versions of loops with this metadata.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:12
@@ +11,3 @@
+// Most of the time when alias analysis is unsure about memory access and it
+// assumes may-alias. This un surety from alias analysis restrict LICM to
+// proceed further. Cases where alias analysis is unsure we like to use loop
----------------
un surety -> uncertainty

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:12
@@ +11,3 @@
+// Most of the time when alias analysis is unsure about memory access and it
+// assumes may-alias. This un surety from alias analysis restrict LICM to
+// proceed further. Cases where alias analysis is unsure we like to use loop
----------------
hfinkel wrote:
> un surety -> uncertainty
restrict LICM to proceed -> restricts LICM from proceeding

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:13
@@ +12,3 @@
+// assumes may-alias. This un surety from alias analysis restrict LICM to
+// proceed further. Cases where alias analysis is unsure we like to use loop
+// versioning as an alternative.
----------------
Cases -> In cases

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:13
@@ +12,3 @@
+// assumes may-alias. This un surety from alias analysis restrict LICM to
+// proceed further. Cases where alias analysis is unsure we like to use loop
+// versioning as an alternative.
----------------
hfinkel wrote:
> Cases -> In cases
like to -> will

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:16
@@ +15,3 @@
+//
+// Loop Versioning will creates version of the loop with aggressive alias and
+// the other with conservative (default) alias. Aggressive alias version of
----------------
creates version -> create a version

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:16
@@ +15,3 @@
+//
+// Loop Versioning will creates version of the loop with aggressive alias and
+// the other with conservative (default) alias. Aggressive alias version of
----------------
hfinkel wrote:
> creates version -> create a version
alias -> aliasing assumptions

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:17
@@ +16,3 @@
+// Loop Versioning will creates version of the loop with aggressive alias and
+// the other with conservative (default) alias. Aggressive alias version of
+// loop will have all the memory access marked as no-alias. These two version
----------------
conservative (default) alias -> conservative (default) aliasing assumptions

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:17
@@ +16,3 @@
+// Loop Versioning will creates version of the loop with aggressive alias and
+// the other with conservative (default) alias. Aggressive alias version of
+// loop will have all the memory access marked as no-alias. These two version
----------------
hfinkel wrote:
> conservative (default) alias -> conservative (default) aliasing assumptions
Aggressive alias version -> The version of the loop making aggressive aliasing assumptions

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:18
@@ +17,3 @@
+// the other with conservative (default) alias. Aggressive alias version of
+// loop will have all the memory access marked as no-alias. These two version
+// of loop will be preceded by a memory runtime check.
----------------
access -> accesses

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:18
@@ +17,3 @@
+// the other with conservative (default) alias. Aggressive alias version of
+// loop will have all the memory access marked as no-alias. These two version
+// of loop will be preceded by a memory runtime check.
----------------
hfinkel wrote:
> access -> accesses
version -> versions

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:21
@@ +20,3 @@
+// This runtime check consists of bound checks for all unique memory accessed
+// in loop, and it ensures aliasing of memory. Based on this check result at
+// runtime any of the loop gets executed, if memory is non aliased then
----------------
ensures aliasing of memory -> ensures the lack of memory aliasing

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:24
@@ +23,3 @@
+// aggressive aliasing loop gets executed, else when memory is aliased then non
+// aggressive aliased version gets executed.
+//
----------------
I'd reword this whole sentence, as: If the runtime check detects any memory aliasing, then the original loop is executed. Otherwise, the version with aggressive aliasing assumptions is used.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:249
@@ +248,3 @@
+    DEBUG(dbgs()
+          << "    loop control flow is not understood by LoopVersioningLICM\n");
+    return false;
----------------
I think this would be better as:
  << "    loop is not bottom tested\n");

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:253
@@ +252,3 @@
+  // Loop depth more then LoopDepthThreshold are not allowed
+  if (CurLoop->getLoopDepth() > LoopDepthThreshold) {
+    DEBUG(dbgs() << "    loop depth is more then threshold\n");
----------------
This is fine for now. I suspect that eventually we'll want to deal with this by versioning the whole loop nest instead of just the inner loop.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:343
@@ +342,3 @@
+  if (PointerSet.size() <= 1) {
+    DEBUG(dbgs() << "    Didnt found sufficient pointers in loop body\n");
+    return false;
----------------
Didnt found -> Didn't find

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:363
@@ +362,3 @@
+  // Check function call safety
+  if(dyn_cast<CallInst>(I) && !AA->doesNotAccessMemory(CallSite(I))) {
+    DEBUG(dbgs() << "    Unsafe call site found.\n");
----------------
if( -> if (

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:373
@@ +372,3 @@
+  // Check parallel loop
+  const bool IsAnnotatedParallel = CurLoop->isAnnotatedParallel();
+  // If current instruction is load instructions
----------------
Parallel loops must not have aliasing loop-invariant memory accesses, or else they'd not be trivially vectorizable. We don't need to version anything in this case, but rather, we should be able to hoist any invariant access. If we don't already get this case, then don't get it here (we don't need to version the loop), but rather, we should handle this directly in the usual LICM pass.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:375
@@ +374,3 @@
+  // If current instruction is load instructions
+  // make sure it's a simple load(non atomic & non volatile)
+  if (I->mayReadFromMemory()) {
----------------
load( -> load (

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:390
@@ +389,3 @@
+  // If current instruction is store instruction
+  // make sure it's a simple store(non atomic & non volatile)
+  else if (I->mayWriteToMemory()) {
----------------
store( -> store (

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:444
@@ +443,3 @@
+  float InvariantPercentage =
+      ((float)(InvariantCounter) / (float)(LoadAndStoreCounter)) * 100;
+  if (InvariantPercentage < InvariantThreshold) {
----------------
Please avoid the use of floating-point computation here (we don't necessarily compile LLVM with strict IEEE support, so we need to be careful to make sure that decision-making procedures are not sensitive to small accuracy changes).

  InvariantPercentage < InvariantThreshold

can become:

  InvariantCounter*100 < InvariantThreshold*LoadAndStoreCounter

(or something like that)


Repository:
  rL LLVM

http://reviews.llvm.org/D9151