[VPlan] Explicitly handle scalar pointer inductions. #83068

fhahn · 2024-02-26T22:04:16Z

Add a new PtrAdd opcode to VPInstruction that corresponds to
IRBuilder::CreatePtrAdd, which creates a GEP with source element type
i8.

This is then used to model scalarizing VPWidenPointerInductionRecipe by
introducing scalar-steps to model the index increment followed by a
PtrAdd.

Note that PtrAdd needs to be able to generate code for only the first
lane or for all lanes. This may warrant introducing a separate recipe
for scalarizing that can be created without relying on the underlying
IR.

Depends on #80271

A VPInstruction only has its first lane used if all users use its first lane only. Use vputils::onlyFirstLaneUsed to continue checking the recipe's users to handle more cases. Besides allowing additional introduction of scalar steps when interleaving in some cases, this also enables using an Add VPInstruction to model the increment.

At the moment, some VPInstructions create only a single scalar value, but use VPTransformatState's 'vector' storage for this value. Those values are effectively uniform-per-VF (or in some cases uniform-across-VF-and-UF). Using the vector/per-part storage doesn't interact well with other recipes, that more accurately using (Part, Lane) to look up scalar values and prevents VPInstructions creating scalars from interacting with other recipes working with scalars. This PR tries to unify handling of scalars by using (Part, 0) for scalar values where only the first lane is demanded. This allows using VPInstructions with other recipes like VPScalarCastRecipe and is also needed when using VPInstructions in more cases otuside the vector loop region to generate scalars. The patch is still a bit rough around the edges, but hopefully serves as start for a discussion how to model more scalar recipes. A potential alternative would be to split off the opcodes that generate scalars only to a separate recipe.

Add a new PtrAdd opcode to VPInstruction that corresponds to IRBuilder::CreatePtrAdd, which creates a GEP with source element type i8. This is then used to model scalarizing VPWidenPointerInductionRecipe by introducing scalar-steps to model the index increment followed by a PtrAdd. Note that PtrAdd needs to be able to generate code for only the first lane or for all lanes. This may warrant introducing a separate recipe for scalarizing that can be created without relying on the underlying IR.

…or-ptr-iv-to-scalar

…o-scalar

github-actions · 2024-02-26T22:07:01Z

✅ With the latest revision this PR passed the C/C++ code formatter.

…o-scalar

fhahn · 2024-03-03T21:50:50Z

ping :)

ayalz · 2024-02-28T11:23:50Z

llvm/lib/Transforms/Vectorize/VPlan.cpp

-          continue;
-
+        assert(!WidenPhi->onlyScalarsGenerated(State->VF.isScalable()) &&
+               "recipe only generating scalars should have been replaced");


Suggested change

"recipe only generating scalars should have been replaced");

"recipe generating only scalars should have been replaced");

Updated, thanks!

ayalz · 2024-02-28T11:28:10Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -1164,11 +1165,14 @@ class VPInstruction : public VPRecipeWithIRFlags {
  /// An optional name that can be used for the generated IR instruction.
  const std::string Name;

+  bool generatesScalars() const;


Worth documenting?

Added, thanks!

ayalz · 2024-02-28T11:28:35Z

llvm/lib/Transforms/Vectorize/VPlan.h

  /// Utility method serving execute(): generates a single instance of the
  /// modeled instruction. \returns the generated value for \p Part.
  /// In some cases an existing value is returned rather than a generated
  /// one.
-  Value *generateInstruction(VPTransformState &State, unsigned Part);
+  Value *generatePerPart(VPTransformState &State, unsigned Part);


Worth updating documentation?
generateInstructionPerPart(), generateInstructionPerLane()??

Updated, thanks!

ayalz · 2024-02-28T11:29:40Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  return
+         Opcode == VPInstruction::PtrAdd;


Suggested change

return

Opcode == VPInstruction::PtrAdd;

return Opcode == VPInstruction::PtrAdd;

ayalz · 2024-03-07T08:35:16Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -273,8 +274,27 @@ VPInstruction::VPInstruction(unsigned Opcode,
  assert(isFPMathOp() && "this op can't take fast-math flags");
 }

-Value *VPInstruction::generateInstruction(VPTransformState &State,
-                                          unsigned Part) {
+bool VPInstruction::generatesScalars() const {


nit: worth clarifying, say, by calling it isGeneratesScalars(), to avoid confusion with the other generate*() methods?

Updated to doesGenerateScalars. WDYT?

ayalz · 2024-03-07T12:55:04Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -509,7 +530,19 @@ void VPInstruction::execute(VPTransformState &State) {
  if (hasFastMathFlags())
    State.Builder.setFastMathFlags(getFastMathFlags());
  for (unsigned Part = 0; Part < State.UF; ++Part) {
-    Value *GeneratedValue = generateInstruction(State, Part);
+    if (generatesScalars()) {


Would it be clearer to further distinguish between singular and plural, i.e., generates-a-single-scalar and generates-multiple-scalars? The former case should arguably call State.set(this, P, Part, !IsVector); where IsVector is set to false, rather than degenerate Lane to iterate over [0,1).

Separated while still calling generatePerLane, thanks!

ayalz · 2024-03-07T12:57:16Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+          vputils::onlyFirstLaneUsed(this) ? 1 : State.VF.getKnownMinValue();
+      if (getOpcode() == VPInstruction::ComputeReductionResult)


Can be folded to assign NumLanes once.

Would it help to fold onlyFirstLaneUsed(this) with opcode == ComputeReductionResult, here and below, and if so - how would it be called - onlySingleLaneDefined?

updated, thanks!

ayalz · 2024-03-07T13:44:39Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

+/// If all users of VPWidenPointerInductionRecipe only use its scalar values,
+/// replace it with a PtrAdd (IndStart, ScalarIVSteps (0, Step)).


This sounds more like a mandatory, functional legalization of induction recipes, rather than an optional, performance optimization - referring to the optimizeInductions() name.

Furthermore, this conceptually introduces two types or stages of the recipes - before and after legalization - which could be represented as two distinct recipes/opcodes, or by recording an indicator whether the recipe was legalized or not. Although these seem unneeded atm.

Yes this is now required (similar to adjusting reductions). Should we move it to a separate transform or possibly clarify the name of the function?

Clarifying the name of the function sounds fine to me.

Updated name and comments

ayalz · 2024-03-07T15:26:21Z

llvm/test/Transforms/LoopVectorize/AArch64/masked-call.ll

@@ -116,8 +116,7 @@ define void @test_widen(ptr noalias %a, ptr readnone %b) #4 {
 ; TFA_INTERLEAVE-NEXT:    [[TMP18:%.*]] = getelementptr inbounds i64, ptr [[TMP15]], i64 [[TMP17]]
 ; TFA_INTERLEAVE-NEXT:    call void @llvm.masked.store.nxv2i64.p0(<vscale x 2 x i64> [[TMP13]], ptr [[TMP15]], i32 8, <vscale x 2 x i1> [[ACTIVE_LANE_MASK]])
 ; TFA_INTERLEAVE-NEXT:    call void @llvm.masked.store.nxv2i64.p0(<vscale x 2 x i64> [[TMP14]], ptr [[TMP18]], i32 8, <vscale x 2 x i1> [[ACTIVE_LANE_MASK2]])
-; TFA_INTERLEAVE-NEXT:    [[INDEX_NEXT:%.*]] = add i64 [[INDEX]], [[TMP6]]
-; TFA_INTERLEAVE-NEXT:    [[INDEX_NEXT4]] = add i64 [[INDEX]], [[TMP6]]
+; TFA_INTERLEAVE-NEXT:    [[INDEX_NEXT]] = add i64 [[INDEX]], [[TMP6]]


The patches associated with this patch mostly move code around, but sometimes helps eliminate more redundancies? Anything worth adding regarding potential 'functional' changes?

Those are spurious changes that were left over from some unrelated changes that I moved out of this patch, restored original version of this file.

ayalz · 2024-03-07T15:28:22Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

+      PtrIV->replaceAllUsesWith(Recipe);
+      continue;
+    }
+


Perhaps worth moving here some of the documentation above that described what happens next.

Added a comment, thanks!

…o-scalar

fhahn · 2024-03-10T12:58:26Z

ping :)

ayalz · 2024-03-10T14:28:27Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  /// Returns true if this VPInstruction generates scalar values only.
+  bool doesGenerateScalars() const;


nit: add Only at the end, or at the beginning - onlyScalarsGenerated(), as in VPWidenPointerInductionRecipe above?

It's somewhat confusing, given VPInstructions such as ComputeReductionResult that also generate scalar(s) are excluded. See more below.

Updated to doesGeneratePerAllLanes as suggested below.

ayalz · 2024-03-10T14:32:57Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp

@@ -44,6 +44,8 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
    CachedTypes[OtherV] = ResTy;
    return ResTy;
  }
+  case VPInstruction::PtrAdd:


nit: worth asserting and caching the type of the other operand, i.e., join the above cases of ICmp and FOR Splice?

PtrAdd's operand have different types, with the first one being a pointer and the second one being an integer offset.

Ah, right, of course. Perhaps worth a comment.

Added, thanks!

ayalz · 2024-03-10T14:43:07Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

    if (!hasResult())
      continue;
    assert(GeneratedValue && "generateInstruction must produce a value");

    bool IsVector = GeneratedValue->getType()->isVectorTy();
    State.set(this, GeneratedValue, Part, !IsVector);
-    assert((IsVector || getOpcode() == VPInstruction::ComputeReductionResult ||
-            State.VF.isScalar() || vputils::onlyFirstLaneUsed(this)) &&
+    assert((IsVector || OnlyFirstLaneDefined || State.VF.isScalar()) &&
           "scalar value but not only first lane used");


Suggested change

"scalar value but not only first lane used");

"scalar value but not only first lane defined");

updated, thanks!

ayalz · 2024-03-10T14:44:41Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+      continue;
+    }
+
+    Value *GeneratedValue = generatePerPart(State, Part);
    if (!hasResult())
      continue;
    assert(GeneratedValue && "generateInstruction must produce a value");

    bool IsVector = GeneratedValue->getType()->isVectorTy();


Confusion above cont'd: if (doesGenerateScalars()) above took care of scalars, IsVector here is expected to be true. But for ComputeReductionResult, e.g..

Updated to use OnlyGenerateFirstLane; in those cases a scalar must be returned and set.

ayalz · 2024-03-10T17:20:19Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+        vputils::onlyFirstLaneUsed(this) ||
+        getOpcode() == VPInstruction::ComputeReductionResult;
+    if (doesGenerateScalars()) {
+      if (OnlyFirstLaneDefined) {


Perhaps something like the following would be clearer:

// First deal with generating multiple values - per all lanes. if (doesGeneratePerAllLanes()) { for (unsigned Lane = 0, NumLanes = State.VF.getKnownMinValue(); Lane != NumLanes; ++Lane) { Value *P = generatePerLane(State, VPIteration(Part, Lane)); State.set(this, P, VPIteration(Part, Lane)); } continue; } // Now deal with generating a single value - per first lane or per part. Value *GeneratedValue = DoesGeneratePerFirstLaneOnly ? generatePerLane(State, VPIteration(Part, 0)) : generatePerPart(State, Part); ...

?

Updated roughly as suggested. also updated to check if only a scalar is needed for the first lane and a scalar can be generated (set to OnlyGenerateFirstLane).

This allows delegating to generatePerPart from generatePerLane, for cases not handled there (all except PtrAdd). This is a consequence of having ocpodes that may generate a vector or scalar per part, sharing codegen for both.

This delegation may raise confusion, worth an explanation, or a clearer alternative?

ayalz · 2024-03-10T23:19:21Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

-Value *VPInstruction::generateInstruction(VPTransformState &State,
-                                          unsigned Part) {
+bool VPInstruction::doesGenerateScalars() const {
+  return Opcode == VPInstruction::PtrAdd;


This is a bit confusing: PtrAdd is (currently) the only VPInstruction that may generate multiple scalars - per lane, or a single scalar - per lane zero only, but there are other VPInstructions that generate a single scalar - per part.

Perhaps doesGeneratePerLane() would be more accurate, or doesGeneratePerAllLanes()? See below.

Updated to use doesGeneratePerAllLanes.

ayalz · 2024-03-10T23:38:12Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

+/// If all users of VPWidenPointerInductionRecipe only use its scalar values,
+/// replace it with a PtrAdd (IndStart, ScalarIVSteps (0, Step)).


Clarifying the name of the function sounds fine to me.

ayalz · 2024-03-10T23:46:19Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

+    }
+
+    // Replace widened induction with scalar steps for users that only use
+    // scalars.


Would be good if these two cases that createScalarIVSteps for scalar users only, would share something.

I think try to share createScalarIVSteps for both would make things more complicated, as they have a lot of different arguments. left as is for now.

…o-scalar

nikolaypanchenko · 2024-03-13T16:24:40Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -1155,6 +1155,10 @@ class VPInstruction : public VPRecipeWithIRFlags {
    BranchOnCount,
    BranchOnCond,
    ComputeReductionResult,
+    // Add an offset in bytes (second operand) to a base pointer (first
+    // operand). Only generates scalar valuse (either for the first lane only or


Fixed, thanks!

nikolaypanchenko · 2024-03-13T18:19:57Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

  for (unsigned Part = 0; Part < State.UF; ++Part) {
-    Value *GeneratedValue = generateInstruction(State, Part);
+    if (doesGeneratePerAllLanes()) {


even though it's not expensive check, but is worth to hoist it out

Moved, thanks!

nikolaypanchenko · 2024-03-13T18:22:29Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

-/// If any user of a VPWidenIntOrFpInductionRecipe needs scalar values,
-/// provide them by building scalar steps off of the canonical scalar IV and
+/// Legalize VPWidenPointer induction, by replacing it with a PtrAdd (IndStart,
+/// ScalarIVSteps (0, Step)) if only its scalar values are used. Als optimize


Adjusted, thanks!

nikolaypanchenko · 2024-03-13T18:26:00Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

  HeaderVPBB->insert(Steps, IP);
  return Steps;
 }

-/// If any user of a VPWidenIntOrFpInductionRecipe needs scalar values,
-/// provide them by building scalar steps off of the canonical scalar IV and
+/// Legalize VPWidenPointer induction, by replacing it with a PtrAdd (IndStart,


Can you please elaborate "legalization" part ? Is it because VPWidenPointerInductionRecipe with onlyScalarsGenerated == true cannot be handled by generation part ?

Yes, VPWidenPointerInductionRecipe can only generate vector values after this change.

I put up #85688 to add some more initial info about this kind of gradual lowering to the docs. What's still missing is a proper name/terminology for recipes that cannot be code-gen'd like predicated VPReplicateRecipe before introducing replicate regions and VPWidenPointerInductionRecipe after this change

…o-scalar

fhahn

Updated to current main and addressed comments, thanks!

fhahn · 2024-03-18T20:03:00Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -1155,6 +1155,10 @@ class VPInstruction : public VPRecipeWithIRFlags {
    BranchOnCount,
    BranchOnCond,
    ComputeReductionResult,
+    // Add an offset in bytes (second operand) to a base pointer (first
+    // operand). Only generates scalar valuse (either for the first lane only or


Fixed, thanks!

fhahn · 2024-03-18T20:03:58Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

  for (unsigned Part = 0; Part < State.UF; ++Part) {
-    Value *GeneratedValue = generateInstruction(State, Part);
+    if (doesGeneratePerAllLanes()) {


Moved, thanks!

fhahn · 2024-03-18T20:11:39Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

-/// If any user of a VPWidenIntOrFpInductionRecipe needs scalar values,
-/// provide them by building scalar steps off of the canonical scalar IV and
+/// Legalize VPWidenPointer induction, by replacing it with a PtrAdd (IndStart,
+/// ScalarIVSteps (0, Step)) if only its scalar values are used. Als optimize


Adjusted, thanks!

fhahn · 2024-03-18T20:13:59Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

  HeaderVPBB->insert(Steps, IP);
  return Steps;
 }

-/// If any user of a VPWidenIntOrFpInductionRecipe needs scalar values,
-/// provide them by building scalar steps off of the canonical scalar IV and
+/// Legalize VPWidenPointer induction, by replacing it with a PtrAdd (IndStart,


Yes, VPWidenPointerInductionRecipe can only generate vector values after this change.

I put up #85688 to add some more initial info about this kind of gradual lowering to the docs. What's still missing is a proper name/terminology for recipes that cannot be code-gen'd like predicated VPReplicateRecipe before introducing replicate regions and VPWidenPointerInductionRecipe after this change

ayalz · 2024-03-11T17:14:12Z

llvm/lib/Transforms/Vectorize/VPlanAnalysis.cpp

@@ -44,6 +44,8 @@ Type *VPTypeAnalysis::inferScalarTypeForRecipe(const VPInstruction *R) {
    CachedTypes[OtherV] = ResTy;
    return ResTy;
  }
+  case VPInstruction::PtrAdd:


Ah, right, of course. Perhaps worth a comment.

ayalz · 2024-03-11T17:18:57Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

-/// If any user of a VPWidenIntOrFpInductionRecipe needs scalar values,
-/// provide them by building scalar steps off of the canonical scalar IV and
+/// Legalize VPWidenPointer induction, by replacing it with a PtrAdd (IndStart,
+/// ScalarIVSteps (0, Step)) if only its scalar values are used. Als optimize


Suggested change

/// ScalarIVSteps (0, Step)) if only its scalar values are used. Als optimize

/// ScalarIVSteps (0, Step)) if only its scalar values are used. Also optimize

ayalz · 2024-03-11T17:19:51Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

+/// Legalize VPWidenPointer induction, by replacing it with a PtrAdd (IndStart,
+/// ScalarIVSteps (0, Step)) if only its scalar values are used. Als optimize
+/// VPWidenIntOrFpInductionRecipe, if any of its users needs scalar values, by
+/// providing them by building scalar steps off of the canonical scalar IV and


Suggested change

/// providing them by building scalar steps off of the canonical scalar IV and

/// providing them scalar steps built on the canonical scalar IV and

Adjusted, thanks!

ayalz · 2024-03-21T15:36:04Z

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp

-/// provide them by building scalar steps off of the canonical scalar IV and
+/// Legalize VPWidenPointerInductionRecipe, by replacing it with a PtrAdd
+/// (IndStart, ScalarIVSteps (0, Step)) if only its scalar values are used, as
+/// VPWidenPointerInductionRecipe cannot generate scalars. Also optimize


Suggested change

/// VPWidenPointerInductionRecipe cannot generate scalars. Also optimize

/// VPWidenPointerInductionRecipe will generate vectors only. Also optimize

So if scalar are used along with vectors of widen pointer induction, the former will be extracted from the latter; as opposed to widening int or fp inductions, where scalar users are fed scalar steps even if there are also vector users. Right? This preserves current behavior, but worth noting the discrepancy, potentially removing it later.

tried to clarify, thanks!

ayalz · 2024-03-21T21:35:50Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

@@ -511,17 +550,33 @@ void VPInstruction::execute(VPTransformState &State) {
         "Recipe not a FPMathOp but has fast-math flags?");
  if (hasFastMathFlags())
    State.Builder.setFastMathFlags(getFastMathFlags());
+  State.Builder.SetCurrentDebugLocation(getDebugLoc());
+  bool OnlyGenerateFirstLane =


Suggested change

bool OnlyGenerateFirstLane =

bool GeneratesPerFirstLaneOnly =

to be more consistent?

Done, thanks!

ayalz · 2024-03-21T21:40:05Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

-    assert((IsVector || getOpcode() == VPInstruction::ComputeReductionResult ||
-            State.VF.isScalar() || vputils::onlyFirstLaneUsed(this)) &&
-           "scalar value but not only first lane used");
+    State.set(this, GeneratedValue, Part, /*IsScalar*/ OnlyGenerateFirstLane);


nit: better to set State after the following assert.

Order flipped, thanks!

ayalz · 2024-03-21T21:43:56Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+    return P;
+  }
+  default: {
+    Value *Res = generatePerPart(State, Lane.Part);


Ah, this raises an eyebrow, or two.
When/is it hard for the caller to accurately call for generatePerLane, rather than generatePerPart? If the former is caller for all lanes, this may end up generating VF copies per same part. Could some assert guard against this from happening, e.g., Lane must be zero?

This is a consequence of generatePerLane being responsible to generate all lanes and/or the first lane, while generatePerPart can either generate the vector or scalar per-part, but would need to also support PtrAdd. It seems clearer to flip around the delegation and have generatePerPart call generatePerLane for PtrAdd. Updated.

ayalz · 2024-03-21T21:49:51Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+    Value *Cmp =
+        Builder.CreateICmp(CmpInst::Predicate::ICMP_UGT, ScalarTC, Step);


still visible, missing a commit?

ayalz · 2024-03-21T21:50:17Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  }
+  default: {
+    Value *Res = generatePerPart(State, Lane.Part);
+    assert(!hasResult() || !Res->getType()->isVectorTy());


missing error message.

This assert is taken care of by the caller of generatePerPart(); could this be done consistently?

Assert is gone now.

ayalz · 2024-03-22T08:12:11Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+        vputils::onlyFirstLaneUsed(this) ||
+        getOpcode() == VPInstruction::ComputeReductionResult;
+    if (doesGenerateScalars()) {
+      if (OnlyFirstLaneDefined) {


This delegation may raise confusion, worth an explanation, or a clearer alternative?

…o-scalar

github-actions · 2024-03-25T19:52:01Z

✅ With the latest revision this PR passed the Python code formatter.

ayalz

This LGTM, thanks! Adding last minor comments and suggestions.

ayalz · 2024-03-25T20:29:28Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  case VPInstruction::PtrAdd:
+    assert(vputils::onlyFirstLaneUsed(this) &&
+           "can only generate first lane for PtrAdd");
+    return generatePerLane(State, VPIteration(Part, 0));


Would this be more consistent:

Value *Ptr = State.get(getOperand(0), Part, /* IsScalar */ true); Value *Addend = State.get(getOperand(1), Part, /* IsScalar */ true); return Builder.CreatePtrAdd(Ptr, Addend, Name);

?

adjusted, thanks!

ayalz · 2024-03-26T07:20:16Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+                                      const VPIteration &Lane) {
+  IRBuilderBase &Builder = State.Builder;
+
+  assert(getOpcode() == VPInstruction::PtrAdd);


nit: error message.

added, thanks!

ayalz · 2024-03-26T07:22:41Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  bool GeneratesPerFirstLaneOnly =
+      canGenerateScalarForFirstLane() &&
+      (vputils::onlyFirstLaneUsed(this) ||
+       getOpcode() == VPInstruction::ComputeReductionResult);


(Unrelated to this patch, while we're here: why is ComputeReductionResult an exception?)

ayalz · 2024-03-26T07:26:04Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+    if (GeneratesPerAllLanes) {
+      for (unsigned Lane = 0, NumLanes = State.VF.getKnownMinValue();
+           Lane != NumLanes; ++Lane) {
+        Value *P = generatePerLane(State, VPIteration(Part, Lane));


Can assert P here, consistent with asserts of GeneratedValue below.

Added, thanks!

ayalz · 2024-03-26T07:27:42Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+      continue;
+    }
+
+    Value *GeneratedValue = generatePerPart(State, Part);
    if (!hasResult())
      continue;
    assert(GeneratedValue && "generateInstruction must produce a value");


generatePerPart rather than generateInstruction.

Fixed, thanks!

ayalz · 2024-03-26T08:18:45Z

llvm/lib/Transforms/Vectorize/VPlan.h

-  /// Utility method serving execute(): generates a single instance of the
-  /// modeled instruction. \returns the generated value for \p Part.
-  /// In some cases an existing value is returned rather than a generated
+  /// Returns true if this VPInstruction generates scalar values for all lanes.


Adding some context? E.g.,
// Most VPInstructions generate a single value per part, either vector or scalar. VPReplicateRecipe takes care of generating multiple (scalar) values per all lanes, stemming from an original ingredient. This method identifies the (rare) cases of VPInstructions that do so as well, w/o an underlying ingredient.

Added, thanks!

…o-scalar

Following up on llvm#83068, when scalarizing VPWidenPointerInductionRecipe, The `onlyScalarsGenerated` checks whether VF is scalable. With scalable VF, it requires all user to use the first lane only. However if any user happens to be VPLiveOut, the check inevitably fails. This patch addresses this by implementing onlyFirstLaneUsed for the VPLiveOut class. It ensures that if the operand is a VPWidenPointerInductionRecipe, it returns true.

fhahn added 17 commits February 1, 2024 10:39

Merge branch 'main' into users/fhahn/vplan-uniform-scalar-lanes

f56e217

!fixup fix merge

172dbf6

Merge branch 'main' into users/fhahn/vplan-uniform-scalar-lanes

d2c51ec

Merge branch 'main' into users/fhahn/vplan-uniform-scalar-lanes

82d74df

!fixup address latest comments, thanks!

c6797e6

!fixup fix formatting

53f2937

Merge branch 'users/fhahn/vplan-uniform-scalar-lanes' into vplan-vect…

b0a78f6

…or-ptr-iv-to-scalar

!fixup Address latest comments, thanks!

e6d2db8

!Fixup split generateInstruction into per-part and per lane.

5065331

!fixup address comments in VPlanTransforms.cpp, thanks!

f38d682

Merge branch 'main' into users/fhahn/vplan-uniform-scalar-lanes

a166da5

Merge branch 'users/fhahn/vplan-uniform-scalar-lanes' into vplan-vect…

df9cad0

…or-ptr-iv-to-scalar

Merge remote-tracking branch 'origin/main' into vplan-vector-ptr-iv-t…

ab14184

…o-scalar

!fixup fix things after update to main.

133776f

fhahn requested review from ayalz and aniragil February 26, 2024 22:04

fhahn mentioned this pull request Feb 26, 2024

[VPlan] Explicitly handle scalar pointer inductions. #80273

Closed

fhahn added 2 commits March 1, 2024 14:05

Merge remote-tracking branch 'origin/main' into vplan-vector-ptr-iv-t…

d8173fb

…o-scalar

!fixup fix formatting.

0bb9f5c

ayalz reviewed Mar 7, 2024

View reviewed changes

fhahn added 2 commits March 7, 2024 21:21

Merge remote-tracking branch 'origin/main' into vplan-vector-ptr-iv-t…

3a698c0

…o-scalar

!fixup address latest comments, thanks!

1e41111

ayalz reviewed Mar 10, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into vplan-vector-ptr-iv-t…

8d05e99

…o-scalar

fhahn added 2 commits March 11, 2024 13:56

fixup address comments.

6f4516f

Merge remote-tracking branch 'origin/main' into vplan-vector-ptr-iv-t…

5f4e4aa

…o-scalar

nikolaypanchenko reviewed Mar 13, 2024

View reviewed changes

fhahn added 2 commits March 18, 2024 19:56

Merge remote-tracking branch 'origin/main' into vplan-vector-ptr-iv-t…

c936a4e

…o-scalar

!fixup address latest comments, thanks!

a9df1d9

fhahn commented Mar 18, 2024

View reviewed changes

ayalz reviewed Mar 22, 2024

View reviewed changes

fhahn added 2 commits March 25, 2024 19:21

Merge remote-tracking branch 'origin/main' into vplan-vector-ptr-iv-t…

9f68460

…o-scalar

!fixup address comments, thansk!

74cb095

ayalz approved these changes Mar 26, 2024

View reviewed changes

fhahn added 2 commits March 26, 2024 13:46

Merge remote-tracking branch 'origin/main' into vplan-vector-ptr-iv-t…

4211565

…o-scalar

!fixup address latest comments, thanks!

643969c

fhahn merged commit 06bb8c9 into llvm:main Mar 26, 2024

fhahn deleted the vplan-vector-ptr-iv-to-scalar branch March 26, 2024 15:02

arcbbb mentioned this pull request May 28, 2024

[VPlan] Implementation of onlyFirstLaneUsed for VPLiveOut class #93513

Closed

fhahn mentioned this pull request Jun 12, 2024

[VPlan] Delay adding canonical IV increment. #82270

Open

	"recipe only generating scalars should have been replaced");
	"recipe generating only scalars should have been replaced");

	return
	Opcode == VPInstruction::PtrAdd;
	return Opcode == VPInstruction::PtrAdd;

		vputils::onlyFirstLaneUsed(this) ? 1 : State.VF.getKnownMinValue();
		if (getOpcode() == VPInstruction::ComputeReductionResult)

		/// If all users of VPWidenPointerInductionRecipe only use its scalar values,
		/// replace it with a PtrAdd (IndStart, ScalarIVSteps (0, Step)).

		/// Returns true if this VPInstruction generates scalar values only.
		bool doesGenerateScalars() const;

	"scalar value but not only first lane used");
	"scalar value but not only first lane defined");

	/// ScalarIVSteps (0, Step)) if only its scalar values are used. Als optimize
	/// ScalarIVSteps (0, Step)) if only its scalar values are used. Also optimize

	/// providing them by building scalar steps off of the canonical scalar IV and
	/// providing them scalar steps built on the canonical scalar IV and

	/// VPWidenPointerInductionRecipe cannot generate scalars. Also optimize
	/// VPWidenPointerInductionRecipe will generate vectors only. Also optimize

	bool OnlyGenerateFirstLane =
	bool GeneratesPerFirstLaneOnly =

		Value *Cmp =
		Builder.CreateICmp(CmpInst::Predicate::ICMP_UGT, ScalarTC, Step);

[VPlan] Explicitly handle scalar pointer inductions. #83068

[VPlan] Explicitly handle scalar pointer inductions. #83068

Uh oh!

Conversation

fhahn commented Feb 26, 2024

Uh oh!

github-actions bot commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fhahn commented Mar 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fhahn commented Mar 10, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

github-actions bot commented Feb 26, 2024 •

edited

Loading