[PATCH] D128755: [VPlan] Make sure optimizeInductions removes wide ind from scalar plan.

[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 fix is fine, thanks, adding a minor nit.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:384
   VPBasicBlock *HeaderVPBB = Plan.getVectorLoopRegion()->getEntryBasicBlock();
+  bool HasScalarVF = Plan.hasVF(ElementCount::getFixed(1));
   for (VPRecipeBase &Phi : HeaderVPBB->phis()) {
----------------
Ah, perhaps HasVectorVF = !Plan


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:389
+        (all_of(IV->users(), [IV](VPUser *U) { return !U->usesScalars(IV); }) &&
+         !NeedsScalarSteps))
       continue;
----------------
fhahn wrote:
> Ayal wrote:
> > Worth reversing the condition, to early-continue if (!(IV && (NeedsScalarSteps || any_of(usesScalars(IV))?
> > (BTW, none_of() seems clearer than all_of(!)).
> > Worth reversing the condition, to early-continue if (!(IV && (NeedsScalarSteps || any_of(usesScalars(IV))?
> > (BTW, none_of() seems clearer than all_of(!)).
> 
> I moved the ScarlarVF check first, thanks! I also updated the check to use `none_of` and also moved out the `!IV` check to a separate if which hopefully should make the code a bit easier to follow.
>> Worth reversing the condition, to early-continue if (!(IV && (NeedsScalarSteps || any_of(usesScalars(IV))?
>> (BTW, none_of() seems clearer than all_of(!)).

> I moved the ScarlarVF check first, thanks! I also updated the check to use none_of and also moved out the !IV check to a separate if which hopefully should make the code a bit easier to follow.

Thanks!
nit: seems logical to combine `HasScalarVF || any_of(usesScalars()`, because a scalar VF implies all use scalars. and then reverse it, w/o applying De Morgan, here and below. But if you prefer the current form is also ok.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128755