[PATCH] D9151: Loop Versioning for LICM

[Hal Finkel via llvm-commits]

hfinkel added a comment.

In http://reviews.llvm.org/D9151#309587, @ashutosh.nema wrote:

> Thanks Hal, for again looking into this change.
>
> > 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?
>
>
> Runtime check also perform overlap checks between non-invariant 
>  accesses. without checking all memory access we can’t make aggressive 
>  aliasing assumptions.


In that case, you should also add llvm.mem.parallel_loop_access metadata to the versioned loop. Otherwise, the vectorizer will add a duplicate set of checks should it decide to vectorize the versiond loop. We also need to add metadata to the original loop to disable vectorization (as we already know that the vectorization checks will have failed when that loop is reached).


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:109
@@ +108,3 @@
+    "loop-versioning-licm", cl::init(false), cl::Hidden,
+    cl::desc("Enable the new, experimental Loop Versioning LICM pass"));
+
----------------
new, experimental -> experimental

(If something is experimental, and yet not new, something's probably wrong ;) )

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:5
@@ +4,3 @@
+//
+// Cases where memory aliasing wasn't sure compiler became conservative and
+// didn't proceeds with invariant code motion. This results in possible
----------------
Remove this paragraph.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:9
@@ +8,3 @@
+//
+// Our main motivation is to exploit such cases and allow LICM optimization.
+//
----------------
Remove this line (it, and the paragraph above, are not needed because the problem is explained clearly in the next paragraph).

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:62
@@ +61,3 @@
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
----------------
These lines need to appear directly under the "The LLVM Compiler Infrastructure" line.

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:135
@@ +134,3 @@
+  MDNode *Node = MDNode::get(C, Elts);
+  I->setMetadata(MDString, Node);
+}
----------------
This utility function is not right, we need to combine the loop metadata here with any other loop metadata which might also exist.

Please see the writeHintsToMetadata function in lib/Transforms/Vectorize/LoopVectorize.cpp and/or the relevant code in lib/Transforms/Utils/LoopUnrollRuntime.cpp near the end of the CloneLoopBlocks function.

As a general note, we really should refactor this into a common utility function.


================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:154
@@ +153,3 @@
+    AU.addRequired<LoopAccessAnalysis>();
+  }
+
----------------
Add here:

    AU.addPreserved<AAResultsWrapperPass>();
    AU.addPreserved<GlobalsAAWrapperPass>();

(so that you don't kill off GlobalsAA here).

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:310
@@ +309,3 @@
+    Value *SomePtr = AS.begin()->getValue();
+    bool typeCheck = true;
+    // Check for Mod & MayAlias
----------------
typeCheck -> TypeCheck

================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:323
@@ +322,3 @@
+    // At least one alias tracker should have pointers of same data type.
+    TypeSafety |= typeCheck;
+  }
----------------
You're checking here that all points in the given alias set have the same type; why?


Repository:
  rL LLVM

http://reviews.llvm.org/D9151