[PATCH] D99750: [LV, VP]VP intrinsics support for the Loop Vectorizer

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

Thanks for the update. Some more comments inline. Mostly small suggestions, but there's one question if masked mem ops are handled correctly and a clarification about active vs effective vector length.



================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h:84
+  bool hasActiveVectorLength(unsigned Opcode, Type *DataType,
+                             Align Alignment) const;
+
----------------
ABataev wrote:
> Ayal wrote:
> > Better called `hasEffectiveVectorLength()` as in hasEVL - for consistency?
> > From the summary: "We use active vector length and explicit vector length interchangeably" - why? Could only EVL be used, consistently? (RISC-V documentation of setvl has AVL as a parameter and [E]VL as return/set value.)
> > 
> > Add Opcode/DataType/Alignment parameters later - when used?
> This is separate. This function is already introduced in TTI, the purpose of the patch is not related to TTI directly.
> As to params, it is done in the implementation.
Just following up on this, should this the name be changed in TTI? Do you know the reason for referring to it has active vector length there vs effective vector length in the patch?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9634
         Value *VectorGep = State.get(getAddr(), Part);
         NewSI = Builder.CreateMaskedScatter(StoredVal, VectorGep, Alignment,
                                             MaskPart);
----------------
Is the gather scatter case handled correctly for EVL at the moment?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2140
+/// A recipe for generating the phi node for the current index of elements,
+/// adjusted in accordance with EVL value.
+class VPEVLBasedIVPHIRecipe : public VPHeaderPHIRecipe {
----------------
Might be good to explicit say that it starts at 0 and gets incremented by EVL in each iteration of the vector loop?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:338
+  case VPInstruction::ExplicitVectorLength: {
+    // Set EVL
+    auto GetSetVL = [=](VPTransformState &State, Value *EVL) {
----------------
nit: `Compute EVL.` would be more accurate IIUC, nothing gets set in this function AFACIT.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:371
+           "EVL type must be smaller than Phi type.");
+    EVL = Builder.CreateIntCast(EVL, Phi->getType(), /*isSigned=*/false);
+    return Builder.CreateAdd(Phi, EVL, Name, hasNoUnsignedWrap(),
----------------
might be good to have a test case where the induction is `i32` and no cast is needed.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1730
+  Value *Start = State.get(getOperand(0), VPIteration(0, 0));
+  PHINode *EntryPart = State.Builder.CreatePHI(Start->getType(), 2, "evl");
+  EntryPart->addIncoming(Start, VectorPH);
----------------
`evl.based.iv`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:1739
+    raw_ostream &O, const Twine &Indent, VPSlotTracker &SlotTracker) const {
+  O << Indent << "EXPLICIT-VECTOR-LANE-PHI ";
+
----------------
This still needs updating I think


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:991
+  }
+  if (none_of(WideCanonicalIV->definedValues(),
+              [](VPValue *V) { return V->getNumUsers(); }))
----------------
`VPWidenCanonicalIVRecipe` should inherit from VPValue via VPHeaderPHIRecipe, so it should only define a single value and `WideCanonicalIV->getNumUsers()` should be sufficient.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1028
 
-    assert(CompareToReplace->getOperand(0) == WideCanonicalIV &&
-           "WidenCanonicalIV must be the first operand of the compare");
-    CompareToReplace->replaceAllUsesWith(LaneMask->getVPSingleValue());
-    CompareToReplace->eraseFromParent();
-  }
+// Add a VPExplicitVectorLengthPHIRecipe and related recipes to \p Plan and
+// replaces all uses except the canonical IV increment of VPCanonicalIVPHIRecipe
----------------
needs updating with new name


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1030
+// replaces all uses except the canonical IV increment of VPCanonicalIVPHIRecipe
+// with a VPExplicitVectorLengthPHIRecipe. VPCanonicalIVPHIRecipe is used only
+// for loop iterations counting after this transformation.
----------------
needs updating with new name


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1062
+  replaceHeaderPredicateWithIdiom(Plan, *VPTrueMask);
+  // Now create the ExplicitVectorLengthPhi recipe in the main loop.
+  auto *EVLPhi = new VPEVLBasedIVPHIRecipe(StartV, DebugLoc());
----------------
name needs updating


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:1080
+  // Replace all uses of VPCanonicalIVPHIRecipe by
+  // VPExplicitVectorLengthPHIRecipe except for
+  // VPInstruction::CanonicalIVIncrement.
----------------
name needs updating


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:84
+  /// replaces all uses except the canonical IV increment of
+  /// VPCanonicalIVPHIRecipe with a VPExplicitVectorLengthPHIRecipe.
+  /// VPCanonicalIVPHIRecipe is used only for loop iterations counting after
----------------
`VPExplicitVectorLengthPHIRecipe` needs updating with new name


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.h:85
+  /// VPCanonicalIVPHIRecipe with a VPExplicitVectorLengthPHIRecipe.
+  /// VPCanonicalIVPHIRecipe is used only for loop iterations counting after
+  /// this transformation.
----------------
maybe `only used to control the loop` or something like that


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanVerifier.cpp:210
+  const VPlan *Plan = VPBB->getPlan();
+  if (Plan && Plan->getEntry()->getNumSuccessors() == 1) {
+    Header = Plan->getVectorLoopRegion()->getEntry();
----------------
`Plan` here should never be `nullptr`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99750