[PATCH] D116123: [VPlan] Handle IV vector splat as VPInstruction.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8442
+        IV = Plan->getOrAddVPValue(Legal->getPrimaryInduction());
+      }
       BlockMask = Builder.createNaryOp(VPInstruction::ActiveLaneMask, {IV});
----------------
fhahn wrote:
> fhahn wrote:
> > fhahn wrote:
> > > Ayal wrote:
> > > > No need for {}
> > > > 
> > > > So a StepVector is needed only to feed ICmpULE, not ActiveLaneMask which depends on first lane only? Seems like an optimization best applied separately?
> > > > 
> > > > BTC is no longer used by ActiveLaneMask, so setting it should sink below to where ICmpULE needs it (While we're here, irrespective of this patch).
> > > > No need for {}
> > > 
> > > I think if the body spans multiple lines due to a comment, braces should not be omitted
> > > https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
> > > 
> > > > So a StepVector is needed only to feed ICmpULE, not ActiveLaneMask which depends on first lane only? Seems like an optimization best applied separately?
> > > 
> > > Yes, let me try to split that off. `ActiveLaneMask` only needs the first scalar step of each `Part`.
> > > 
> > > > BTC is no longer used by ActiveLaneMask, so setting it should sink below to where ICmpULE needs it (While we're here, irrespective of this patch).
> > > 
> > > done separately in 2e630eabd329
> > >> So a StepVector is needed only to feed ICmpULE, not ActiveLaneMask which depends on first lane only? Seems like an optimization best applied separately?
> > >Yes, let me try to split that off. ActiveLaneMask only needs the first scalar step of each Part.
> > 
> > I just double checked and we may still need to go the route with the widened recipe if there's no primary induction. But there's no test case without primary inductions that use active.lane.mask. I;ll push one and update the patch.
> I missed something here and IIUC your comment was regarding creating a step vector in both cases at first, right?
> 
> The issue with that is that in some cases `ActiveLaneMask` will use the step-vector instead of the scalar steps, so I left it as is for now.
> 
> > I just double checked and we may still need to go the route with the widened recipe if there's no primary induction. 
> 
> The current patch still creates the vector phi when necessary and the existing tests cover that already.
Yes, my comment refers to the fact that currently both ActiveLaneMask and ICmpULE are treated the same when Legal has a PrimaryInduction() - both use the same IV = Plan->getOrAddVPValue(Legal->getPrimaryInduction());
This patch continues to feed ActiveLaneMask with this IV, but feeds ICmpULE with a StepVector.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8849
+        return VF.isVector() && ShouldScalarizeInstruction(Instr, VF) &&
+               NeedsScalarInduction(VF);
+      },
----------------
fhahn wrote:
> Ayal wrote:
> > NeedsScalarInduction(VF) will always early-return true given that ShouldScalarizeInstruction(Instr, VF) is true?
> I think it is required for the case where `Instr` is a truncate.
Doesn't `ShouldScalarizeInstruction(Instr, VF) && NeedsScalarInduction(VF)` always equals `ShouldScalarizeInstruction(Instr, VF)` given that `NeedsScalarInduction(VF)` returns true if `ShouldScalarizeInstruction(Instr, VF)` returns true for the same Instr and VF?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116123