[VPlan] Add VPInst::getNumOperandsForOpcode, use to verify in ctor (NFC) #142284

fhahn · 2025-05-31T20:23:08Z

Add a new getNumOperandsForOpcode helper to determine the number of operands from the opcode. For now, it is used to verify the number operands at VPInstruction construction.

It returns -1 for a few opcodes where the number of operands cannot be determined (GEP, Switch, PHI, Call).

This can also be used in a follow-up to determine if a VPInstruction is masked based on the number of arguments.

Add a new getNumOperandsForOpcode helper to determine the number of operands from the opcode. For now, it is used to verify the number operands at VPInstruction construction. It returns -1 for a few opcodes where the number of operands cannot be determined (GEP, Switch, PHI, Call). This can also be used in a follow-up to determine if a VPInstruction is masked based on the number of arguments.

llvmbot · 2025-05-31T20:23:40Z

@llvm/pr-subscribers-vectorizers

@llvm/pr-subscribers-llvm-transforms

Author: Florian Hahn (fhahn)

Changes

Add a new getNumOperandsForOpcode helper to determine the number of operands from the opcode. For now, it is used to verify the number operands at VPInstruction construction.

It returns -1 for a few opcodes where the number of operands cannot be determined (GEP, Switch, PHI, Call).

This can also be used in a follow-up to determine if a VPInstruction is masked based on the number of arguments.

Full diff: https://github.com/llvm/llvm-project/pull/142284.diff

2 Files Affected:

(modified) llvm/lib/Transforms/Vectorize/VPlan.h (+7)
(modified) llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp (+58-1)

diff --git a/llvm/lib/Transforms/Vectorize/VPlan.h b/llvm/lib/Transforms/Vectorize/VPlan.h
index 44f0b6d964a6e..bd6a3247abd7d 100644
--- a/llvm/lib/Transforms/Vectorize/VPlan.h
+++ b/llvm/lib/Transforms/Vectorize/VPlan.h
@@ -967,6 +967,13 @@ class VPInstruction : public VPRecipeWithIRFlags,
   /// value for lane \p Lane.
   Value *generatePerLane(VPTransformState &State, const VPLane &Lane);
 
+#if !defined(NDEBUG)
+  /// Return the number of operands determined by the opcode of the
+  /// VPInstruction. Returns -1 if the number of operands cannot be determined
+  /// directly by the opcode.
+  unsigned getNumOperandsForOpcode() const;
+#endif
+
 public:
   VPInstruction(unsigned Opcode, ArrayRef<VPValue *> Operands, DebugLoc DL = {},
                 const Twine &Name = "")
diff --git a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
index a4831ea7c11f7..5c6e4aeaf3cad 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
+++ b/llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
@@ -413,8 +413,62 @@ VPInstruction::VPInstruction(unsigned Opcode, ArrayRef<VPValue *> Operands,
       Opcode(Opcode), Name(Name.str()) {
   assert(flagsValidForOpcode(getOpcode()) &&
          "Set flags not supported for the provided opcode");
+  assert((getNumOperandsForOpcode() == -1u ||
+          getNumOperandsForOpcode() == getNumOperands()) &&
+         "number of operands does not match opcode");
 }
 
+#ifndef NDEBUG
+unsigned VPInstruction::getNumOperandsForOpcode() const {
+  if (Instruction::isUnaryOp(getOpcode()) || Instruction::isCast(getOpcode()))
+    return 1;
+
+  if (Instruction::isBinaryOp(getOpcode()))
+    return 2;
+
+  switch (getOpcode()) {
+  case VPInstruction::StepVector:
+    return 0;
+  case Instruction::Alloca:
+  case Instruction::ExtractValue:
+  case Instruction::Freeze:
+  case Instruction::Load:
+  case VPInstruction::AnyOf:
+  case VPInstruction::BranchOnCond:
+  case VPInstruction::CalculateTripCountMinusVF:
+  case VPInstruction::CanonicalIVIncrementForPart:
+  case VPInstruction::ExplicitVectorLength:
+  case VPInstruction::ExtractLastElement:
+  case VPInstruction::ExtractPenultimateElement:
+  case VPInstruction::FirstActiveLane:
+  case VPInstruction::Not:
+    return 1;
+
+  case Instruction::ICmp:
+  case Instruction::FCmp:
+  case Instruction::Store:
+  case VPInstruction::ActiveLaneMask:
+  case VPInstruction::BranchOnCount:
+  case VPInstruction::ComputeReductionResult:
+  case VPInstruction::FirstOrderRecurrenceSplice:
+  case VPInstruction::LogicalAnd:
+  case VPInstruction::WideIVStep:
+  case VPInstruction::PtrAdd:
+    return 2;
+  case Instruction::Select:
+  case VPInstruction::ComputeFindLastIVResult:
+    return 3;
+  case Instruction::Call:
+  case Instruction::PHI:
+  case Instruction::GetElementPtr:
+  case Instruction::Switch:
+    // Cannot determine the number of operands from the opcode.
+    return -1u;
+  }
+  llvm_unreachable("all cases should be handled above");
+}
+#endif
+
 bool VPInstruction::doesGeneratePerAllLanes() const {
   return Opcode == VPInstruction::PtrAdd && !vputils::onlyFirstLaneUsed(this);
 }
@@ -2706,7 +2760,10 @@ static void scalarizeInstruction(const Instruction *Instr,
 
   // Replace the operands of the cloned instructions with their scalar
   // equivalents in the new loop.
-  for (const auto &I : enumerate(RepRecipe->operands())) {
+  auto OpRange = RepRecipe->operands();
+  if (isa<CallBase>(Cloned))
+    OpRange = drop_end(OpRange);
+  for (const auto &I : enumerate(OpRange)) {
     auto InputLane = Lane;
     VPValue *Operand = I.value();
     if (vputils::isSingleScalar(Operand))

ayalz · 2025-06-04T12:49:01Z

llvm/lib/Transforms/Vectorize/VPlan.h

+  /// Return the number of operands determined by the opcode of the
+  /// VPInstruction. Returns -1 if the number of operands cannot be determined
+  /// directly by the opcode.
+  unsigned getNumOperandsForOpcode() const;


static or vputils getNumOperandsFor(Opcode)?

Done thanks!

ayalz · 2025-06-04T12:51:37Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  case VPInstruction::WideIVStep:
+  case VPInstruction::PtrAdd:


Suggested change

case VPInstruction::WideIVStep:

case VPInstruction::PtrAdd:

case VPInstruction::PtrAdd:

case VPInstruction::WideIVStep:

Flipped, thanks

ayalz · 2025-06-04T12:52:01Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  case Instruction::PHI:
+  case Instruction::GetElementPtr:


Suggested change

case Instruction::PHI:

case Instruction::GetElementPtr:

case Instruction::GetElementPtr:

case Instruction::PHI:

Flipped, thanks!

ayalz · 2025-06-04T12:53:29Z

llvm/lib/Transforms/Vectorize/VPlan.h

@@ -967,6 +967,13 @@ class VPInstruction : public VPRecipeWithIRFlags,
  /// value for lane \p Lane.
  Value *generatePerLane(VPTransformState &State, const VPLane &Lane);

+#if !defined(NDEBUG)
+  /// Return the number of operands determined by the opcode of the
+  /// VPInstruction. Returns -1 if the number of operands cannot be determined


Suggested change

/// VPInstruction. Returns -1 if the number of operands cannot be determined

/// VPInstruction. Returns -1u if the number of operands cannot be determined

Done thanks

ayalz · 2025-06-04T13:02:21Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  auto OpRange = RepRecipe->operands();
+  if (isa<CallBase>(Cloned))
+    OpRange = drop_end(OpRange);
+  for (const auto &I : enumerate(OpRange)) {


How/is this related to introducing getNumOperandsForOpcode() (i.e., rest of patch)

This was unrelated, removed ,thanks

…foropcode

ayalz · 2025-06-20T21:00:47Z

This can also be used in a follow-up to determine if a VPInstruction is masked based on the number of arguments.

BTW, masking could alternatively have an explicit API for setting and getting, possibly maintaining a pointer to a VPInstruction's mask or null when unmasked.

…foropcode

fhahn · 2025-06-21T09:50:38Z

This can also be used in a follow-up to determine if a VPInstruction is masked based on the number of arguments.

BTW, masking could alternatively have an explicit API for setting and getting, possibly maintaining a pointer to a VPInstruction's mask or null when unmasked.

Yep, the WIP patch to use this for masking provides such an API, although based on the number of operands (https://github.com/llvm/llvm-project/pull/142285/files#diff-dabd8bf9956285f5ddab712af4e0494647a106234f07283b0bd9f8b4d9b887caR1039-R1073)

I think the mask operand still needs to be part of the operands of the VPInstruction, to make sure it is part of the def-use graph, to get updated automatically when the mask value gets RAUWs.

ayalz · 2025-06-22T18:01:08Z

This can also be used in a follow-up to determine if a VPInstruction is masked based on the number of arguments.

BTW, masking could alternatively have an explicit API for setting and getting, possibly maintaining a pointer to a VPInstruction's mask or null when unmasked.

Yep, the WIP patch to use this for masking provides such an API, although based on the number of operands (https://github.com/llvm/llvm-project/pull/142285/files#diff-dabd8bf9956285f5ddab712af4e0494647a106234f07283b0bd9f8b4d9b887caR1039-R1073)

I think the mask operand still needs to be part of the operands of the VPInstruction, to make sure it is part of the def-use graph, to get updated automatically when the mask value gets RAUWs.

Agreed, the mask should be a regular operand, and in addition have a setter and getter to set and retrieve it.

ayalz

LGTM, minor nits.

ayalz · 2025-06-22T18:03:40Z

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp

+  case VPInstruction::FirstActiveLane:
+  case VPInstruction::Not:
+    return 1;
+


Suggested change

consistency

Done thanks

ayalz · 2025-06-22T18:17:43Z

llvm/unittests/Transforms/Vectorize/VPlanTest.cpp

@@ -706,13 +706,15 @@ TEST_F(VPBasicBlockTest, reassociateBlocks) {

 #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
 TEST_F(VPBasicBlockTest, print) {
-  VPInstruction *TC = new VPInstruction(Instruction::Add, {});
+  VPInstruction *TC = new VPInstruction(Instruction::PHI, {});


To escape num operands check?
Considered scalar, w/o operands.

Yes. The tests here just check the various print implementations, the actual opcodes/operands don't matter much, other than matching the print checks.

There's a chicken-egg problem here, as we need to get a VPValue before calling getPlan.

ayalz · 2025-06-22T18:30:41Z

llvm/unittests/Transforms/Vectorize/VPlanTest.cpp

-  VPInstruction *I1 = new VPInstruction(Instruction::Add, {});
-  VPInstruction *I2 = new VPInstruction(Instruction::Sub, {I1});
+  VPInstruction *I1 = new VPInstruction(Instruction::Add, {Val, Val});
+  VPInstruction *I2 = new VPInstruction(Instruction::Sub, {I1, Val});
  VPInstruction *I3 = new VPInstruction(Instruction::Br, {I1, I2});


Independent: a branch has two VPInstruction operands?

Yep, this just creates artificial VPInstructions to test printing, the ocpodes don't really matter, other than checking.

We should probably tighten down which kinds of opcodes are supported for VPInstruction

ayalz · 2025-06-22T18:40:13Z

llvm/unittests/Transforms/Vectorize/VPlanTest.cpp

-  VPInstruction *I5 = new VPInstruction(Instruction::Ret, {I4});
+  VPInstruction *I5 = new VPInstruction(Instruction::Br, {I4});


Avoid checking return with a single operand? A VPInstruction with an Instruction opcode should retain the latter's number of operands, but that may require checking an underlying Instruction?

Yep, we currently never create VPInstruction for ret instructions in LV, so it seemed better to use FNeg as used above.

…foropcode

… in ctor (NFC) (#142284) Add a new getNumOperandsForOpcode helper to determine the number of operands from the opcode. For now, it is used to verify the number operands at VPInstruction construction. It returns -1 for a few opcodes where the number of operands cannot be determined (GEP, Switch, PHI, Call). This can also be used in a follow-up to determine if a VPInstruction is masked based on the number of arguments. PR: llvm/llvm-project#142284

…FC) (llvm#142284) Add a new getNumOperandsForOpcode helper to determine the number of operands from the opcode. For now, it is used to verify the number operands at VPInstruction construction. It returns -1 for a few opcodes where the number of operands cannot be determined (GEP, Switch, PHI, Call). This can also be used in a follow-up to determine if a VPInstruction is masked based on the number of arguments. PR: llvm#142284

fhahn requested review from rengolin, ayalz and aniragil May 31, 2025 20:23

llvmbot added vectorizers llvm:transforms labels May 31, 2025

fhahn mentioned this pull request May 31, 2025

[VPlan] Support masked VPInsts, use for predication (NFC)(WIP) #142285

Open

ayalz reviewed Jun 4, 2025

View reviewed changes

fhahn added 5 commits June 6, 2025 10:32

Merge remote-tracking branch 'origin/main' into vpinst-getnumoperands…

cd2f291

…foropcode

!fixup address comments, thanks

f58eadd

Merge remote-tracking branch 'origin/main' into vpinst-getnumoperands…

c134f89

…foropcode

!fixup handle updated opcodes.

e78eb44

Merge remote-tracking branch 'origin/main' into vpinst-getnumoperands…

0d8c982

…foropcode

fhahn added 2 commits June 21, 2025 10:06

Merge remote-tracking branch 'origin/main' into vpinst-getnumoperands…

ef1ba88

…foropcode

!fixup update gtests constructing VPInstructions.

d2f5c26

ayalz approved these changes Jun 22, 2025

View reviewed changes

fhahn added 3 commits June 24, 2025 12:01

Merge remote-tracking branch 'origin/main' into vpinst-getnumoperands…

0c8de77

…foropcode

!fixup remove stray newline and adjust opcode in test.

031aeca

!fixup use BR again

f829e30

fhahn merged commit c3e25e7 into llvm:main Jun 24, 2025
7 checks passed

fhahn deleted the vpinst-getnumoperandsforopcode branch June 24, 2025 19:39

	/// VPInstruction. Returns -1 if the number of operands cannot be determined
	/// VPInstruction. Returns -1u if the number of operands cannot be determined

		VPInstruction *I5 = new VPInstruction(Instruction::Ret, {I4});
		VPInstruction *I5 = new VPInstruction(Instruction::Br, {I4});

[VPlan] Add VPInst::getNumOperandsForOpcode, use to verify in ctor (NFC) #142284

[VPlan] Add VPInst::getNumOperandsForOpcode, use to verify in ctor (NFC) #142284

Uh oh!

Conversation

fhahn commented May 31, 2025

Uh oh!

llvmbot commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

ayalz commented Jun 20, 2025

Uh oh!

fhahn commented Jun 21, 2025

Uh oh!

ayalz commented Jun 22, 2025

Uh oh!

ayalz left a comment

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!

Uh oh!

Uh oh!

llvmbot commented May 31, 2025 •

edited

Loading