[PATCH] D9151: Loop Versioning for LICM

[hfinkel@anl.gov via llvm-commits]

hfinkel added a comment.

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

> Thanks Hal for looking into this again.
>
>   I've started to look at this again; I apologize for the delay...
>   
>   ================
>   Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:125
>   @@ +124,3 @@
>   +
>   +// Sets input string as meta data to loop latch terminator instruction.
>   +static void setLoopMetaData(Loop *CurLoop, const char *MDString) {
>   ----------------
>   I see that you've marked this as done; does this patch need to be updated to reflect the change?
>
> This comments was old comment on moving 'cloneLoop' to utility.
>  But now it's no more required as that function got removed, and using LoopVersioning utility for cloning etc.
>
>   ================
>   Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:223
>   @@ +222,3 @@
>   +
>   +/// \brief Check loop structure and confirms it's good for LoopVersioningLICM.
>   +bool LoopVersioningLICM::legalLoopStructure() {
>   ----------------
>   Can you be more specific? Are these checks reflecting limitation of the versioning infrastructure?
>
> Some part of checks are repeated, but I kept them to complete legality at one place.
>  Not sure it's a right decision.
>
>   Heuristics for profitability? Both? Please specifically explain this in the comments for each check.
>
> Sure will update.
>
>   ================
>   Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:291
>   @@ +290,3 @@
>   +  // 2) PointerSet shouldn't have pointers more then RuntimeMemoryCheckThreshold
>   +  // 3) Make sure AliasSets doesn't have any must alias set.
>   +  for (const auto &I : *CurAST) {
>   ----------------
>   Why? Must-alias with what? Why does it matter if there happens to be something that aliases with something else, if there is a third thing that only may alias with the first two?
>
> Case where 2 pointers are must-aliased, there runtime bound check always give same result. In such cases there is no need for runtime checks and loop versioning.


Alright, I have a better understanding of what you're doing now. Your technique for generating the versioned loop is to check that all pointer access in the loop are independent (and, thus, don't alias), and guarded by that check, you reach the versioned variant of the loop where the aliasing metadata asserts that all access are mutually independent. The follow-up LICM pass then actually does the hoisting.

I agree this makes sense, because if you had multiple aliasing domains then you'd still not necessarily be able to hoist the potentially-loop-invariant accesses out of the loop. Please, however, explain all of this near the CurAST iteration code so that it is clear why you have this restriction.

> 

> 

>   ================

>   Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:308

>   @@ +307,3 @@

>   +      Value *Ptr = A.getValue();

>   +      // alias tracker should have pointers of same data type.

>   +      typeCheck = (typeCheck && (SomePtr->getType() == Ptr->getType()));

>   ----------------

>   alias -> Alias

> 

> Will correct.

> 

>   ================

>   Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:333

>   @@ +332,3 @@

>   +  }

>   +  // Atleast 2 pointers needed for runtime check.

>   +  if (PointerSet.size() <= 1) {

>   ----------------

>   Atleast -> At least

> 

> Will correct.

> 

>   ================

>   Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:356

>   @@ +355,3 @@

>   +  const bool IsAnnotatedParallel = CurLoop->isAnnotatedParallel();

>   +  // We dont support call instructions. however, we ignore few intrinsic

>   +  // and libfunc callsite. We don't allow non-intrinsic, non-libfunc callsite

>   ----------------

>   Why not? I understand that the vectorizer has these checks, but I don't see why this belongs in an LICM pass?

> 

> There is a possibility that call may modify aliasing behavior, which may defeat the purpose of versioning & runtime checks.


Ah, good point. However, please then replace this check with an appropriate AA getModRef-type check for whether the call might alias with the relevant pointers (if the function, for example, does not alias with any of the loop-invariant accesses, then it can't affect what you're trying to do, although you'll need to be somewhat more careful about adding the noalias metadata to those functions, etc.).

>   Repository:

>     rL LLVM

>    

>   http://reviews.llvm.org/D9151



================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:97
@@ +96,3 @@
+#define DEBUG_TYPE "loop-versioning-licm"
+#define ORIGINAL_LOOP_METADATA "LoopVersioningLICMOriginalLoop"
+#define VERSION_LOOP_METADATA "LoopVersioningLICMVersionLoop"
----------------
please use metadata names more consistent with our general naming scheme for standard metadata (llvm.loop.whatever). You should need only one metadata type, to disable licm-based versioning, it should be documented in the LangRef, and you can add it to both original and versioned loops once this pass has run.



================
Comment at: lib/Transforms/Scalar/LoopVersioningLICM.cpp:426
@@ +425,3 @@
+  }
+  // Loop should have at least invariant store instruction.
+  if (!InvariantCounter) {
----------------
Should comment say "load or store instruction"?


Repository:
  rL LLVM

http://reviews.llvm.org/D9151