[PATCH] D116123: [VPlan] Handle IV vector splat using VPWidenCanonicalIV.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This is fine, with a last nit



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:356
+  if (Range.Start.isScalar())
+    Range.End = Range.Start * 2;
+
----------------
fhahn wrote:
> Ayal wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > Is this optimization expected to clamp Range.End for the current VPlan during VPlans-for-ranges construction?
> > > > Is there a need to (only) know if VPlan's range is scalar? Specifically, if Start is Scalar then End can be asserted to be Start*2, if needed? Suffice the check if all recipes provide scalars only, although faster to check if range is scalar?
> > > It's sufficient to check whether the start is scalar. There's no need to clamp the end.
> > Does the following summarize the logic here: Original and New IV's can each provide either scalar only, vector only, or both. Original can replace New iff it provides whatever New needs to provide.
> > 
> > Have VPWidenCanonicalIVRecipe also support needsScalarIVOnly() - by checking if all it's users are ActiveLaneMask VPInstructions, and needsScalarIV() - by checking if any user is ActiveLaneMask?
> > 
> > Then check
> > 
> > ```
> >   if ((WidenNewIV->needsScalarIV() && WidenOriginalIV->needsScalarIV()) ||
> >       (!WidenNewIV->needsScalarIVOnly() && !WidenOriginalIV->needsScalarIVOnly())) {
> >       WidenNewIV->replaceAllUsesWith(WidenOriginalIV);
> >       WidenNewIV->eraseFromParent();
> >   }
> > ```
> > 
> > ?
> > Does the following summarize the logic here: Original and New IV's can each provide either scalar only, vector only, or both. Original can replace New iff it provides whatever New needs to provide.
> 
> Yes, that's a better summary! I updated the comment.
> 
> I am not sure about adding `needsScalarIVOnly`/`needsScalarIV` to `VPWidenCanonicalIVRecipe`, as this distinction seems only relevant here and might muddy the waters a bit, as the recipe will always generate a vector value (and I don't think we should change that). I left the check here as is for now, but once D116554 lands this check can be replaced with `onlyFirstLaneUsed(WidenNewIV)`. 
> 
> I think to wrap things up, it is worth to keep the current order of patches and have the temporary check of `WidenNewIV`'s users here, to simplify the patch ordering.
Very well.
How about just setting here

```
  bool WidenNewIVOnlyFirstLaneUsed = all_of(...);
  if (WidenOriginalIV->needsVectorIV() || WidenNewIVOnlyFirstLaneUsed) {
    WidenNewIV->replaceAllUsesWith(WidenOriginalIV);
    WidenNewIV->eraseFromParent();
  }
```
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116123