[VPlan] Use parameter packs to avoid unary/binary/ternary matchers. NFC #152272
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Instead of defining unary/binary/ternary/4ary overloads of each matcher, we can use parameter packs to support arbitrary numbers of operands. This allows us to remove some otherwise unused definitions like UnaryRecipe_match and TernaryRecipe_match. We need to rewrite Recipe_match's constructor to use a parameter pack too, otherwise we end up with ambiguous overloads.
Member
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-vectorizers Author: Luke Lau (lukel97) ChangesInstead of defining unary/binary/ternary/4ary overloads of each matcher, we can use parameter packs to support arbitrary numbers of operands. This allows us to remove some otherwise unused definitions like UnaryRecipe_match and TernaryRecipe_match. We need to rewrite Recipe_match's constructor to use a parameter pack too, otherwise we end up with ambiguous overloads. Full diff: https://github.com/llvm/llvm-project/pull/152272.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h b/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
index d133610ef4f75..e6fee7a625efe 100644
--- a/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
+++ b/llvm/lib/Transforms/Vectorize/VPlanPatternMatch.h
@@ -200,15 +200,9 @@ template <typename Ops_t, unsigned Opcode, bool Commutative,
struct Recipe_match {
Ops_t Ops;
- Recipe_match() : Ops() {
- static_assert(std::tuple_size<Ops_t>::value == 0 &&
- "constructor can only be used with zero operands");
- }
- Recipe_match(Ops_t Ops) : Ops(Ops) {}
- template <typename A_t, typename B_t>
- Recipe_match(A_t A, B_t B) : Ops({A, B}) {
- static_assert(std::tuple_size<Ops_t>::value == 2 &&
- "constructor can only be used for binary matcher");
+ template <typename... OpTy> Recipe_match(OpTy... Ops) : Ops(Ops...) {
+ static_assert(std::tuple_size<Ops_t>::value == sizeof...(Ops) &&
+ "number of operands in constructor doesn't match Ops_t");
}
bool match(const VPValue *V) const {
@@ -274,42 +268,19 @@ template <unsigned Opcode, typename... RecipeTys>
using ZeroOpRecipe_match =
Recipe_match<std::tuple<>, Opcode, false, RecipeTys...>;
-template <typename Op0_t, unsigned Opcode, typename... RecipeTys>
-using UnaryRecipe_match =
- Recipe_match<std::tuple<Op0_t>, Opcode, false, RecipeTys...>;
-
-template <typename Op0_t, unsigned Opcode>
-using UnaryVPInstruction_match =
- UnaryRecipe_match<Op0_t, Opcode, VPInstruction>;
-
template <unsigned Opcode>
using ZeroOpVPInstruction_match = ZeroOpRecipe_match<Opcode, VPInstruction>;
template <typename Op0_t, unsigned Opcode>
using AllUnaryRecipe_match =
- UnaryRecipe_match<Op0_t, Opcode, VPWidenRecipe, VPReplicateRecipe,
- VPWidenCastRecipe, VPInstruction>;
+ Recipe_match<std::tuple<Op0_t>, Opcode, /*Commutative*/ false, VPWidenRecipe, VPReplicateRecipe,
+ VPWidenCastRecipe, VPInstruction>;
template <typename Op0_t, typename Op1_t, unsigned Opcode, bool Commutative,
typename... RecipeTys>
using BinaryRecipe_match =
Recipe_match<std::tuple<Op0_t, Op1_t>, Opcode, Commutative, RecipeTys...>;
-template <typename Op0_t, typename Op1_t, unsigned Opcode>
-using BinaryVPInstruction_match =
- BinaryRecipe_match<Op0_t, Op1_t, Opcode, /*Commutative*/ false,
- VPInstruction>;
-
-template <typename Op0_t, typename Op1_t, typename Op2_t, unsigned Opcode,
- bool Commutative, typename... RecipeTys>
-using TernaryRecipe_match = Recipe_match<std::tuple<Op0_t, Op1_t, Op2_t>,
- Opcode, Commutative, RecipeTys...>;
-
-template <typename Op0_t, typename Op1_t, typename Op2_t, unsigned Opcode>
-using TernaryVPInstruction_match =
- TernaryRecipe_match<Op0_t, Op1_t, Op2_t, Opcode, /*Commutative*/ false,
- VPInstruction>;
-
template <typename Op0_t, typename Op1_t, unsigned Opcode,
bool Commutative = false>
using AllBinaryRecipe_match =
@@ -322,70 +293,42 @@ inline ZeroOpVPInstruction_match<VPInstruction::BuildVector> m_BuildVector() {
return ZeroOpVPInstruction_match<VPInstruction::BuildVector>();
}
-template <unsigned Opcode, typename Op0_t>
-inline UnaryVPInstruction_match<Op0_t, Opcode>
-m_VPInstruction(const Op0_t &Op0) {
- return UnaryVPInstruction_match<Op0_t, Opcode>(Op0);
-}
+template <unsigned Opcode, typename... OpTys>
+using VPInstruction_match =
+ Recipe_match<std::tuple<OpTys...>, Opcode, /*Commutative*/ false, VPInstruction>;
-template <unsigned Opcode, typename Op0_t, typename Op1_t>
-inline BinaryVPInstruction_match<Op0_t, Op1_t, Opcode>
-m_VPInstruction(const Op0_t &Op0, const Op1_t &Op1) {
- return BinaryVPInstruction_match<Op0_t, Op1_t, Opcode>(Op0, Op1);
+template <unsigned Opcode, typename... OpTys>
+inline VPInstruction_match<Opcode, OpTys...>
+m_VPInstruction(const OpTys &... Ops) {
+ return VPInstruction_match<Opcode, OpTys...>(Ops...);
}
-template <unsigned Opcode, typename Op0_t, typename Op1_t, typename Op2_t>
-inline TernaryVPInstruction_match<Op0_t, Op1_t, Op2_t, Opcode>
-m_VPInstruction(const Op0_t &Op0, const Op1_t &Op1, const Op2_t &Op2) {
- return TernaryVPInstruction_match<Op0_t, Op1_t, Op2_t, Opcode>(
- {Op0, Op1, Op2});
-}
-
-template <typename Op0_t, typename Op1_t, typename Op2_t, typename Op3_t,
- unsigned Opcode, bool Commutative, typename... RecipeTys>
-using Recipe4Op_match = Recipe_match<std::tuple<Op0_t, Op1_t, Op2_t, Op3_t>,
- Opcode, Commutative, RecipeTys...>;
-
-template <typename Op0_t, typename Op1_t, typename Op2_t, typename Op3_t,
- unsigned Opcode>
-using VPInstruction4Op_match =
- Recipe4Op_match<Op0_t, Op1_t, Op2_t, Op3_t, Opcode, /*Commutative*/ false,
- VPInstruction>;
-
-template <unsigned Opcode, typename Op0_t, typename Op1_t, typename Op2_t,
- typename Op3_t>
-inline VPInstruction4Op_match<Op0_t, Op1_t, Op2_t, Op3_t, Opcode>
-m_VPInstruction(const Op0_t &Op0, const Op1_t &Op1, const Op2_t &Op2,
- const Op3_t &Op3) {
- return VPInstruction4Op_match<Op0_t, Op1_t, Op2_t, Op3_t, Opcode>(
- {Op0, Op1, Op2, Op3});
-}
template <typename Op0_t>
-inline UnaryVPInstruction_match<Op0_t, Instruction::Freeze>
+inline VPInstruction_match<Instruction::Freeze, Op0_t>
m_Freeze(const Op0_t &Op0) {
return m_VPInstruction<Instruction::Freeze>(Op0);
}
template <typename Op0_t>
-inline UnaryVPInstruction_match<Op0_t, VPInstruction::BranchOnCond>
+inline VPInstruction_match<VPInstruction::BranchOnCond, Op0_t>
m_BranchOnCond(const Op0_t &Op0) {
return m_VPInstruction<VPInstruction::BranchOnCond>(Op0);
}
template <typename Op0_t>
-inline UnaryVPInstruction_match<Op0_t, VPInstruction::Broadcast>
+inline VPInstruction_match<VPInstruction::Broadcast, Op0_t>
m_Broadcast(const Op0_t &Op0) {
return m_VPInstruction<VPInstruction::Broadcast>(Op0);
}
template <typename Op0_t, typename Op1_t>
-inline BinaryVPInstruction_match<Op0_t, Op1_t, VPInstruction::ActiveLaneMask>
+inline VPInstruction_match<VPInstruction::ActiveLaneMask, Op0_t, Op1_t>
m_ActiveLaneMask(const Op0_t &Op0, const Op1_t &Op1) {
return m_VPInstruction<VPInstruction::ActiveLaneMask>(Op0, Op1);
}
template <typename Op0_t, typename Op1_t>
-inline BinaryVPInstruction_match<Op0_t, Op1_t, VPInstruction::BranchOnCount>
+inline VPInstruction_match<VPInstruction::BranchOnCount, Op0_t, Op1_t>
m_BranchOnCount(const Op0_t &Op0, const Op1_t &Op1) {
return m_VPInstruction<VPInstruction::BranchOnCount>(Op0, Op1);
}
@@ -486,7 +429,7 @@ m_Select(const Op0_t &Op0, const Op1_t &Op1, const Op2_t &Op2) {
}
template <typename Op0_t>
-inline match_combine_or<UnaryVPInstruction_match<Op0_t, VPInstruction::Not>,
+inline match_combine_or<VPInstruction_match<VPInstruction::Not, Op0_t>,
AllBinaryRecipe_match<int_pred_ty<is_all_ones>, Op0_t,
Instruction::Xor, true>>
m_Not(const Op0_t &Op0) {
@@ -496,7 +439,7 @@ m_Not(const Op0_t &Op0) {
template <typename Op0_t, typename Op1_t>
inline match_combine_or<
- BinaryVPInstruction_match<Op0_t, Op1_t, VPInstruction::LogicalAnd>,
+ VPInstruction_match<VPInstruction::LogicalAnd, Op0_t, Op1_t>,
AllTernaryRecipe_match<Op0_t, Op1_t, specific_intval<1>,
Instruction::Select>>
m_LogicalAnd(const Op0_t &Op0, const Op1_t &Op1) {
@@ -514,7 +457,7 @@ m_LogicalOr(const Op0_t &Op0, const Op1_t &Op1) {
template <typename Op0_t, typename Op1_t, typename Op2_t>
using VPScalarIVSteps_match =
- TernaryRecipe_match<Op0_t, Op1_t, Op2_t, 0, false, VPScalarIVStepsRecipe>;
+ Recipe_match<std::tuple<Op0_t, Op1_t, Op2_t>, 0, false, VPScalarIVStepsRecipe>;
template <typename Op0_t, typename Op1_t, typename Op2_t>
inline VPScalarIVSteps_match<Op0_t, Op1_t, Op2_t>
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
artagnon
reviewed
Aug 6, 2025
Contributor
artagnon
left a comment
artagnon
left a comment
There was a problem hiding this comment.
This is an excellent improvement, thanks!
fhahn
reviewed
Aug 6, 2025
Comment on lines
+203
to
+205
| template <typename... OpTy> Recipe_match(OpTy... Ops) : Ops(Ops...) { | ||
| static_assert(std::tuple_size<Ops_t>::value == sizeof...(Ops) && | ||
| "number of operands in constructor doesn't match Ops_t"); |
Contributor
There was a problem hiding this comment.
Is this change needed? It seems orthogonal to removing the various matchers, at least initially?
Contributor
Author
There was a problem hiding this comment.
Keeping the old constructors around causes ambiguous constructor overloads, and fails to compile without. Also some places seem to construct Recipe_match with the {} syntax which invokes the Recipe_match(Ops_t Ops) constructor, so I had to remove that too.
lukel97
changed the title
lukel97
commented
Aug 6, 2025
| // Check for recipes that do not have opcodes. | ||
| if constexpr (std::is_same<RecipeTy, VPScalarIVStepsRecipe>::value || | ||
| std::is_same<RecipeTy, VPCanonicalIVPHIRecipe>::value || | ||
| std::is_same<RecipeTy, VPWidenSelectRecipe>::value || |
Contributor
Author
There was a problem hiding this comment.
This was removed because VPWidenSelectRecipe was added to AllRecipe_match (to remove TernaryRecipe_match), and we don't want unary or binary matchers to unconditionally match on VPWidenSelectRecipe.
VPWidenSelectRecipe defines getOpcode() to be Instruction::Select so just use that instead.
artagnon
reviewed
Aug 6, 2025
artagnon
approved these changes
Aug 6, 2025
| template <typename... OpTy> Recipe_match(OpTy... Ops) : Ops(Ops...) { | ||
| static_assert(std::tuple_size<Ops_t>::value == sizeof...(Ops) && | ||
| "number of operands in constructor doesn't match Ops_t"); | ||
| static_assert(!(Commutative && std::tuple_size<Ops_t>::value != 2) && |
Contributor
There was a problem hiding this comment.
Suggested change
| static_assert(!(Commutative && std::tuple_size<Ops_t>::value != 2) && | |
| static_assert((!Commutative || std::tuple_size<Ops_t>::value == 2) && |
| UnaryRecipe_match<Op0_t, Opcode, VPInstruction>; | ||
| template <unsigned Opcode, typename... OpTys> | ||
| using AllRecipe_match = | ||
| Recipe_match<std::tuple<OpTys...>, Opcode, false, VPWidenRecipe, |
Contributor
There was a problem hiding this comment.
Suggested change
| Recipe_match<std::tuple<OpTys...>, Opcode, false, VPWidenRecipe, | |
| Recipe_match<std::tuple<OpTys...>, Opcode, /*Commutative*/ false, VPWidenRecipe, |
| using ZeroOpVPInstruction_match = ZeroOpRecipe_match<Opcode, VPInstruction>; | ||
| template <unsigned Opcode, typename... OpTys> | ||
| using AllRecipe_commutative_match = | ||
| Recipe_match<std::tuple<OpTys...>, Opcode, true, VPWidenRecipe, |
Contributor
There was a problem hiding this comment.
Suggested change
| Recipe_match<std::tuple<OpTys...>, Opcode, true, VPWidenRecipe, | |
| Recipe_match<std::tuple<OpTys...>, Opcode, /*Commutative*/ true, VPWidenRecipe, |
| BinaryRecipe_match<Op0_t, Op1_t, Instruction::GetElementPtr, false, | ||
| VPWidenRecipe, VPReplicateRecipe, VPWidenGEPRecipe, | ||
| VPInstruction>; | ||
| Recipe_match<std::tuple<Op0_t, Op1_t>, Instruction::GetElementPtr, false, |
Contributor
There was a problem hiding this comment.
Suggested change
| Recipe_match<std::tuple<Op0_t, Op1_t>, Instruction::GetElementPtr, false, | |
| Recipe_match<std::tuple<Op0_t, Op1_t>, Instruction::GetElementPtr, /*Commutative*/ false, |
Comment on lines
+437
to
+438
| using VPScalarIVSteps_match = Recipe_match<std::tuple<Op0_t, Op1_t, Op2_t>, 0, | ||
| false, VPScalarIVStepsRecipe>; |
Contributor
There was a problem hiding this comment.
Suggested change
| using VPScalarIVSteps_match = Recipe_match<std::tuple<Op0_t, Op1_t, Op2_t>, 0, | |
| false, VPScalarIVStepsRecipe>; | |
| using VPScalarIVSteps_match = Recipe_match<std::tuple<Op0_t, Op1_t, Op2_t>, /*Opcode*/ 0, | |
| /*Commutative*/ false, VPScalarIVStepsRecipe>; |
david-arm
reviewed
Aug 7, 2025
Contributor
david-arm
left a comment
david-arm
left a comment
There was a problem hiding this comment.
Looks like a sensible tidy-up! Just a couple of minor comments/thoughts.
| using UnaryVPInstruction_match = | ||
| UnaryRecipe_match<Op0_t, Opcode, VPInstruction>; | ||
| template <unsigned Opcode, typename... OpTys> | ||
| using AllRecipe_match = |
Contributor
There was a problem hiding this comment.
This is just a thought, but is it worth making this consistent with AllRecipe_commutative_match by renaming it to AllRecipe_noncommutative_match? I realise the naming scheme was already like that before your patch though. At the moment it sounds a bit like it's a superset that includes commutative matches too.
Contributor
Author
There was a problem hiding this comment.
I'm not opposed to this, but I guess I was trying to make it consistent with the names of the actual matchers themselves, i.e m_foo for non-commutative and m_c_foo for commutative.
Would renaming the commutative version to AllRecipe_c_match be more consistent?
Contributor
There was a problem hiding this comment.
Probably best to keep this in-line with IR PatternMatch, for which the default is also non-commutative w/o special naming I think.
| UnaryRecipe_match<Op0_t, Opcode, VPWidenRecipe, VPReplicateRecipe, | ||
| VPWidenCastRecipe, VPInstruction>; | ||
| template <unsigned Opcode, typename... OpTys> | ||
| using VPInstruction_match = Recipe_match<std::tuple<OpTys...>, Opcode, |
Contributor
There was a problem hiding this comment.
Do we not need both commutative and non-commutative variants? We use VPInstruction types for normal IR opcodes too, such as add, mul, etc. which are commutative.
Contributor
Author
There was a problem hiding this comment.
Yeah we could add m_c_VPInstruction at some point, but it didn't exist before this patch. Just doing a quick check though I can't really find any prospective users around. I think most cases that would want something like this use m_c_Binary which matches other non-VPInstruction recipes too.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of defining unary/binary/ternary/4ary overloads of each matcher, we can use parameter packs to support arbitrary numbers of operands.
This allows us to remove the explicit N-ary definitions for each matcher.
We need to rewrite Recipe_match's constructor to use a parameter pack too, otherwise we end up with ambiguous overloads.