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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:384
   VPBasicBlock *HeaderVPBB = Plan.getVectorLoopRegion()->getEntryBasicBlock();
+  bool NeedsScalarSteps = Plan.hasVF(ElementCount::getFixed(1));
   for (VPRecipeBase &Phi : HeaderVPBB->phis()) {
----------------
Ayal wrote:
> How about `AllUseScalars`, or `ScalarVF`? "NeedsScalarSteps" combines the existence of a true usesScalars user together with a scalar VF.
> 
> Worth clarifying that usesScalars() assumes a VF>1 vector is given, as when VF=1 all users are provided a scalar and use it. I.e., "usesScalarsOfAVector()".
> How about AllUseScalars, or ScalarVF? "NeedsScalarSteps" combines the existence of a true usesScalars user together with a scalar VF.

Good point, I updated the variable name to `ScalarVF`.

> Worth clarifying that usesScalars() assumes a VF>1 vector is given, as when VF=1 all users are provided a scalar and use it. I.e., "usesScalarsOfAVector()".

Sounds good, I can update that separately.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:388
     if (!IV ||
-        all_of(IV->users(), [IV](VPUser *U) { return !U->usesScalars(IV); }))
+        (all_of(IV->users(), [IV](VPUser *U) { return !U->usesScalars(IV); }) &&
+         !NeedsScalarSteps))
----------------
bmahjour wrote:
> suggestion:
> ```
> (none_of(IV->users(), [IV](VPUser *U) { return U->usesScalars(IV); })
> ```
Thanks, I wasn't aware of `none_of`. Updated!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:389
+        (all_of(IV->users(), [IV](VPUser *U) { return !U->usesScalars(IV); }) &&
+         !NeedsScalarSteps))
       continue;
----------------
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.


================
Comment at: llvm/test/Transforms/LoopVectorize/interleave-and-scalarize-only.ll:172
+
+
+; DBG-LABEL: 'first_order_recurrence_using_induction'
----------------
Ayal wrote:
> Explain what this test is designed to check? (Worth pre-committing it to demonstrate failure?)
Thanks, I added an explanation. Unfortunately pre-committing it would mean we have to XFAIL the whole file, as it causes a crash with assertions :( 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128755