[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/VPlan.cpp:1674
+  });
+}
----------------
Ayal wrote:
> Demanded == Used?
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:75
 
+Value *getRuntimeVFAsFloat(IRBuilder<> &B, Type *FTy, ElementCount VF);
+
----------------
Ayal wrote:
> Place above next to getRuntimeVF()?
Moved up, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:80
+/// to each vector element of Val. The sequence starts at StartIndex.
+/// \p Opcode is relevant for FP induction variable.
+Value *getStepVector(Value *Val, Value *StartIdx, Value *Step,
----------------
Ayal wrote:
> (Indep. of this patch, but while we're here:)
> "The sequence starts at StartIndex." - redundant?
> \p Opcode >> \p BinOp
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:773
+
+  /// Returns true if the recipe only uses the first lane of operand \p Op.
+  virtual bool onlyFirstLaneUsed(const VPValue *Op) const {
----------------
Ayal wrote:
> Default is to return false conservatively.
extended comment.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:780
+
+  /// Returns true if the recipe only uses scalars of operand \p Op.
+  virtual bool onlyScalarsUsed(const VPValue *Op) const {
----------------
Ayal wrote:
> ditto
extended comment.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:926
+           "Op must be an operand of the recipe");
+    return getOperand(0) == Op && getOpcode() == VPInstruction::ActiveLaneMask;
+  }
----------------
Ayal wrote:
> The recipes along the canonical IV chain also use first lane only, namely: CanonicalIVIncrement, CanonicalIVIncrementNUW, BranchOnCount and VPCanonicalIVPHIRecipe? They are also "onlyFirstPartUsed()", but are unique in that.
Updated to include CanonicalIVIncrement, CanonicalIVIncrementNUW, BranchOnCount  opcodes.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1364
+    assert(is_contained(operands(), Op) &&
+           "Op must be an operand of the recipe");
+    return all_of(users(), [this](VPUser *U) {
----------------
Ayal wrote:
> Comment that "// Recursing through Blend recipes only, must terminate at header phi's the latest." ?
Added the comment, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1747
+
+    // Widened, consecutive memory operations only demand the first lane.
+    return isConsecutive();
----------------
Ayal wrote:
> ... of their address.
> 
> `return Op == getAddr() && isConsecutive();` ?
Simplified as suggested, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:342
   VPBasicBlock *HeaderVPBB = Plan.getVectorLoopRegion()->getEntryBasicBlock();
   for (VPRecipeBase &Phi : HeaderVPBB->phis()) {
     auto *WidenOriginalIV = dyn_cast<VPWidenIntOrFpInductionRecipe>(&Phi);
----------------
Ayal wrote:
> Ahh, removeRedundantInductionCasts() is called before hoisting VPWidenIntOrFpInductionRecipes-built-from-Truncs, so the latter will be excluded if we only visit phis() here? OTOH, are such truncated IV's candidates for folding with WidenCanonicalIV in terms of matching types?
> OTOH, are such truncated IV's candidates for folding with WidenCanonicalIV in terms of matching types? 

I don't think so, as we should widen using the canonical IVs type, which would be the wider type.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:354
     // directly.
-    if (WidenOriginalIV && WidenOriginalIV->isCanonical() &&
-        WidenOriginalIV->getScalarType() == WidenNewIV->getScalarType()) {
-      WidenNewIV->replaceAllUsesWith(WidenOriginalIV);
-      WidenNewIV->eraseFromParent();
-      return;
+    // If scalar values are demanded from WidenNewIV, create a stepvector.
+    // TODO: Remove checks once a vector phi will be generated for all
----------------
Ayal wrote:
> The VPWidenCanonicalIVRecipe WidenNewIV is created only to feed the ICmpULE or ActiveLane, i.e., when vectorizing with fold tail, so can assert(!Range.Start.isScalar && FoldTail) if needed below?
> 
> Scalar values are demanded from WidenNewIV only if it feeds ActiveLane, which demands only the first lane, so broadcasting and adding building scalar steps for each lane seems redundant here? Suffice to feed ActiveLane with UF parts each holding first lane only, either constructed via a simplified StepVector from canonicalIV or perhaps have the canonicalIV supply them directly instead of broadcasting its Part 0 value across all parts (which now seems potentially misleading)?
> The VPWidenCanonicalIVRecipe WidenNewIV is created only to feed the ICmpULE or ActiveLane, i.e., when vectorizing with fold tail, so can assert(!Range.Start.isScalar && FoldTail) if needed below?

I might be missing something, but I think It is still possible to fold the tail with VF=1 and UF>1, so I left the check (e.g. in `llvm/test/Transforms/LoopVectorize/pr45679-fold-tail-by-masking.ll`)


> Scalar values are demanded from WidenNewIV only if it feeds ActiveLane, which demands only the first lane, so broadcasting and adding building scalar steps for each lane seems redundant here? Suffice to feed ActiveLane with UF parts each holding first lane only, either constructed via a simplified StepVector from canonicalIV or perhaps have the canonicalIV supply them directly instead of broadcasting its Part 0 value across all parts (which now seems potentially misleading)?

Yes, for ActiveLane we only need the first scalar part of each lane. I think it might be better to have a special version of build-scalar-steps that only generates the first lane as follow-up to the build-scalar-steps patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116123