[PATCH] D96546: [LoopVectorize] NFCI: BuildVPlansWithVPRecipes to include ScalableVFs.

[Nashe Mncube via Phabricator via llvm-commits]

nasherm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1979
+    for (auto VF = MinVF; ElementCount::isKnownLE(VF, MaxVF); VF *= 2) {
+      if (!CM.canVectorizeReductions(VF))
+        return;
----------------
There is a code path that's being travelled down after your genFeasibleVFCandidates function that causes a runtime crash where VF elements
. The issue is caused by the fact that this code path assumes that the supplied MinFactor and MaxFactor, of type ElementCount, will be in the Scalars map. For illustration here's the piece of code that triggers this error code path

```
Optional<VectorizationFactor>
7595 LoopVectorizationPlanner::plan(ElementCount UserVF, unsigned UserIC) {
...
7640   genFeasibleVFCandidates(CM, VFCandidates, MinFactors, MaxFactors);
7641   for (auto &VF : VFCandidates) {
7642     // Collect Uniform and Scalar instructions after vectorization with VF.
7643     CM.collectUniformsAndScalars(VF);
7644 
7645     // Collect the instructions (and their associated costs) that will be more
7646     // profitable to scalarize.
7647     if (VF.isVector())
7648       CM.collectInstsToScalarize(VF);
7649   }
7650 
7651   CM.collectInLoopReductions();
7652 
7653   buildVPlansWithVPRecipes(MinFactors, MaxFactors); // <-- code path starts from this call
7654  ....
```
The buildVPlansWithVPRecipes function will traverse MinFactors->MaxFactors, similar to the genFeasibleVFCandidates candidates, but it will not perform the same !CM.canVectorizeReductions check which allows for the early breakout. As a consequence it will eventually hit this function

```
8374 bool VPRecipeBuilder::shouldWiden(Instruction *I, VFRange &Range) const {
8375   assert(!isa<BranchInst>(I) && !isa<PHINode>(I) && !isa<LoadInst>(I) &&
8376          !isa<StoreInst>(I) && "Instruction should have been handled earlier");
8377   // Instruction should be widened, unless it is scalar after vectorization,
8378   // scalarization is profitable or it is predicated.
8379   auto WillScalarize = [this, I](ElementCount VF) -> bool {
8380     return CM.isScalarAfterVectorization(I, VF) ||
8381            CM.isProfitableToScalarize(I, VF) ||
8382            CM.isScalarWithPredication(I, VF);
8383   };
8384   return !LoopVectorizationPlanner::getDecisionAndClampRange(WillScalarize,
8385                                                              Range);
8386 }
```

The call to CM.isScalarVectorization is the source of the error where the assert call is causing the crash. A simple, but perhaps inelegant solution, would be to add the !CM.canVectorizeReductions to the list of checks.


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

https://reviews.llvm.org/D96546