[PATCH] D103700: [LV] Fix bug when unrolling (only) a loop with non-latch exit

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3175
+  // iterations check ensures that N >= Step.
+  if (Cost->requiresScalarEpilogue()) {
     auto *IsZero = Builder.CreateICmpEQ(R, ConstantInt::get(R->getType(), 0));
----------------
reames wrote:
> Ayal wrote:
> > What if a loop has a single exiting block - the loop latch, and an interleave group that requires a scalar epilog, but we decide to unroll the loop w/o vectorizing it? Such a (test)case should be unrolled w/o a scalar epilog.
> > 
> > Note that InterleaveInfo.requiresScalarEpilogue() is relevant only when vectorizing but is otherwise independent of VF, so should force a scalar epilog in conjunction with the original "if (VF > 1)" of D19487. Exiting from a non-latch block, OTOH, as introduced in D93317, should force a scalar epilog for any VF, including 1.
> > 
> > Also curious if D94892 should be applicable to epilog vectorization, as commented there.
> > What if a loop has a single exiting block - the loop latch, and an interleave group that requires a scalar epilog, but we decide to unroll the loop w/o vectorizing it? Such a (test)case should be unrolled w/o a scalar epilog.
> We could reasonably decide that such a loop does not require a scalar epilogue, but if the cost model decides it does (as it might today), code generation had better be consistent about it.  That's all this patch does.  
> > 
> > Note that InterleaveInfo.requiresScalarEpilogue() is relevant only when vectorizing but is otherwise independent of VF, so should force a scalar epilog in conjunction with the original "if (VF > 1)" of D19487. Exiting from a non-latch block, OTOH, as introduced in D93317, should force a scalar epilog for any VF, including 1.
> I don't follow your comment here.  As demonstrated by the test case, we do need to generate an epilogue loop in some cases even when not vectorizing. 
> > 
> > Also curious if D94892 should be applicable to epilog vectorization, as commented there.
> (will reply there)
> 
> > What if a loop has a single exiting block - the loop latch, and an interleave group that requires a scalar epilog, but we decide to unroll the loop w/o vectorizing it? Such a (test)case should be unrolled w/o a scalar epilog.
> We could reasonably decide that such a loop does not require a scalar epilogue, but if the cost model decides it does (as it might today), code generation had better be consistent about it.  That's all this patch does.  

We do decide rightfully that such a loop does not require a scalar epilogue today; this patch causes it
to have a scalar epilogue.
Take for example test case even_load_static_tc from interleaved-accesses.ll, where instead of "-force-vector-width=4 -force-vector-interleave=1" (which requires an epilog) we run it with "-force-vector-width=1 -force-vector-interleave=4" (which does not require an epilog).

 
> > 
> > Note that InterleaveInfo.requiresScalarEpilogue() is relevant only when vectorizing but is otherwise independent of VF, so should force a scalar epilog in conjunction with the original "if (VF > 1)" of D19487. Exiting from a non-latch block, OTOH, as introduced in D93317, should force a scalar epilog for any VF, including 1.
> I don't follow your comment here.  As demonstrated by the test case, we do need to generate an epilogue loop in some cases even when not vectorizing. 

Sorry if the comment is unclear. The logic for forcing a scalar epilog should conceptually be

```
  ((getExitingBlock() != getLoopLatch()) ||
   (VF.isVector() && InterleaveInfo.requiresScalarEpilogue()))
```
, right?
Asserting that `isScalarEpilogueAllowed()` holds if the above condition is true: because if an epilog is not allowed, Legal should have prevented vectoring a loop with a non-latch exiting block, and interleave groups requiring epilog should not have been formed.

To have `Cost->requiresScalarEpilogue()` fully convey this logic, one could directly pass it VF or VF.isVector(); or reset InterleaveInfo's requiresScalarEpilogue earlier when VF is set to 1, e.g., by invalidating all its groups.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103700/new/

https://reviews.llvm.org/D103700