[PATCH] D9151: Loop Versioning for LICM

[Nema, Ashutosh via llvm-commits]

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.

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


      Repository:
        rL LLVM

      http://reviews.llvm.org/D9151


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150921/5de6bb5c/attachment.html>