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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2566
+  // Create a new independent vector induction variable, if one is needed.
+  if (!Def->needsScalarIVOnly())
     createVectorIntOrFpInductionPHI(ID, Step, Start, EntryVal, Def, State);
----------------
OK, this is synonymous with NeedsVectorIV(), capturing it more accurately.

Follows the above comment:
>Perhaps VPWidenIntOrFpInductionRecipe should know of NeedsScalarIV and NeedsVectorIV (names to be improved), pass them to ILV->widenIntOrFpInduction() and to others - see removeRedundantStepVector below?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:356
+  if (Range.Start.isScalar())
+    Range.End = Range.Start * 2;
+
----------------
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();
  }
```

?


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-sink-replicate-region.ll:175
 ; CHECK-NEXT:   WIDEN-REDUCTION-PHI ir<%and.red> = phi ir<1234>, ir<%and.red.next>
-; CHECK-NEXT:   EMIT vp<[[MASK:%.+]]> = icmp ule ir<%iv> vp<[[BTC]]>
+; CHECK-NEXT:   EMIT vp<[[WIDEN_CAN:%.+]]> = WIDEN-CANONICAL-INDUCTION vp<%2>
+; CHECK-NEXT:   EMIT vp<[[MASK:%.+]]> = icmp ule vp<[[WIDEN_CAN]]> vp<[[BTC]]>
----------------
Should vp<%2> be vp<[[CAN_IV]]> ?


================
Comment at: llvm/test/Transforms/LoopVectorize/pr45679-fold-tail-by-masking.ll:153
 ; VF1UF4:       pred.store.if:
-; VF1UF4-NEXT:    [[TMP4:%.*]] = getelementptr inbounds i32, i32* [[A:%.*]], i32 [[INDUCTION]]
+; VF1UF4-NEXT:    [[SUNK_IND0:%.*]] = add i32 [[INDEX]], 0
+; VF1UF4-NEXT:    [[TMP4:%.*]] = getelementptr inbounds i32, i32* [[A:%.*]], i32 [[SUNK_IND0]]
----------------
Hmm, this enables sink scalar operands to handle scalar steps.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116123