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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8442
+        IV = Plan->getOrAddVPValue(Legal->getPrimaryInduction());
+      }
       BlockMask = Builder.createNaryOp(VPInstruction::ActiveLaneMask, {IV});
----------------
Ayal wrote:
> 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.
> 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());

Ah I see. Yes, currently they both use the same VPValue, because the single recipe creates both the scalar steps and the step vector. But ActiveLaneMask only needs the steps and ICmpULE only needs the step vector. This is explicit now in the code as a result of breaking this part of the phi handling into distinct pieces.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8849
+        return VF.isVector() && ShouldScalarizeInstruction(Instr, VF) &&
+               NeedsScalarInduction(VF);
+      },
----------------
Ayal wrote:
> 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?
I think to preserve the existing behaviour the PHI needs to be scalarized and any of its users. I updated the code to make the check a bit more direct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116123