[PATCH] D50480: [LV] Vectorizing loops of arbitrary trip count without remainder under opt for size

[Hideki Saito via Phabricator via llvm-commits]

hsaito added inline comments.


================
Comment at: include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:485
   SmallPtrSet<const Instruction *, 8> MaskedOp;
+
+  /// All blocks of loop are to be masked to fold tail of scalar iterations.
----------------
Ayal wrote:
> hsaito wrote:
> > I think it's best not to keep this state in the Legal. From the Legal perspective, being able to vectorize the whole loop body under the mask and the actual decision to do so are completely separate issues. 
> > 
> > Since canFold...() is invoked by CostModel::computeMaxVF, we should be able to keep this state in the CostModel. After all, whether to bail out or continue under FoldTailByMasking is a cost model side of the state, after consulting the Legal.
> OK.
Thank you.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:792
 
 bool LoopVectorizationLegality::blockNeedsPredication(BasicBlock *BB) {
+  return (FoldTailByMasking ||
----------------
Ayal wrote:
> hsaito wrote:
> > By moving FoldTail state to CostModel,  we can define CostModel::blockNeedsPredication(BB) as FoldTailByMasking || LAI::blockNeedsPredication(BB) and make Legal version static to Legal.
> > 
> OK, except that LAI::blockNeedsPredication() also asks for DT which CostModel does not have. Let's have CostModel::blockNeedsPredication(BB) return FoldTailByMasking || Legal::blockNeedsPredication(). Hopefully the two will not cause confusion.
> 
> Making Legal version static should be pursued in a separate patch, if desired.
Thanks, and fair enough.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2748
+  // If tail is to be folded, vector loop takes care of all iterations.
+  Value *CheckMinIters = Builder.getFalse();
+  if (!Cost->foldTailByMasking())
----------------
Personally, I don't like to see the IR like the following going out of the vectorizer, even though that's later cleaned up tirivially.
    %1 = false       // unused and thus will be trivially cleaned up later.
    %2 = icmp ...
Changing this part of the patch to
     Value *CheckMinIters = nullptr;
     if ()
         ....
     else
         CheckMinIters = Builder.getFalse();
would make cleaner IR going out for common cases, at a small price to pay in ease-of-reading.

If you agree, great. If not, I won't make a big deal about it. At the end of the day, we should clean up this area of code such that we don't have to rely on CheckMinIters being "false" constant to cleanup the unnecessary min iter check. That improvement can be done as a separate NFC patch.




================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2990
+  // If tail is to be folded, we know we don't need to run the remainder.
+  Value *CmpN = Builder.getTrue();
+  if (!Cost->foldTailByMasking())
----------------
See the comment on CheckMinIters.


https://reviews.llvm.org/D50480