[PATCH] D9151: Loop Versioning for LICM

[Nema, Ashutosh via llvm-commits]

Thanks Hal for review.

I'll incorporate your comments and come back soon.

      >
      >   ================
      >   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.

Sure I'll mention about this.


      >   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.).

I'm not sure that itself would be sufficient, consider indirect calls probably need to consider function pointer in runtime check.
Also there is a possibility that in call argument one of the runtime pointer escaped, in such cases it become more difficult to ensure correctness.
That's why for simplicity I considered vectorizer approach to consider only few functions.


      ================
      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.

Sure I'll use only one meta data.
Will update LangRef as well.

      ================
      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"?

Ah, this comment needs to be updated.


Regards,
Ashutosh


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150928/703d35ad/attachment.html>