[GlobalISel][TableGen] MIR Pattern Variadics #100563
Merged
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
Allow for matching & rewriting a variable number of arguments in an instructions. Solves llvm#87459
|
@llvm/pr-subscribers-llvm-globalisel Author: Pierre van Houtryve (Pierre-vh) ChangesAllow for matching & rewriting a variable number of arguments in an instructions. Solves #87459 Patch is 53.41 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/100563.diff 15 Files Affected:
diff --git a/llvm/docs/GlobalISel/MIRPatterns.rst b/llvm/docs/GlobalISel/MIRPatterns.rst
index 7e6d88683d491..abc23985a6fac 100644
--- a/llvm/docs/GlobalISel/MIRPatterns.rst
+++ b/llvm/docs/GlobalISel/MIRPatterns.rst
@@ -115,7 +115,7 @@ GITypeOf
``GITypeOf<"$x">`` is a ``GISpecialType`` that allows for the creation of a
register or immediate with the same type as another (register) operand.
-Operand:
+Type Parameters:
* An operand name as a string, prefixed by ``$``.
@@ -143,6 +143,57 @@ Semantics:
(apply (G_FSUB $dst, $src, $tmp),
(G_FNEG GITypeOf<"$dst">:$tmp, $src))>;
+GIVariadic
+~~~~~~~~~~
+
+``GIVariadic<>`` is a ``GISpecialType`` that allows for matching 1 or
+more operands remaining on an instruction.
+
+Type Parameters:
+
+* The minimum number of additional operands to match. Must be greater than zero.
+
+ * Default is 1.
+
+* The maximum number of additional operands to match. Must be strictly greater
+ than the minimum.
+
+ * 0 can be used to indicate there is no upper limit.
+ * Default is 0.
+
+Semantics:
+
+* ``GIVariadic<>`` operands can not be defs.
+* ``GIVariadic<>`` operands can only appear as the last operand in a 'match' pattern.
+* Each instance within a 'match' pattern must be uniquely named.
+* Re-using a ``GIVariadic<>`` operand in an 'apply' pattern will result in all
+ the matched operands being copied from the original instruction.
+* The min/max operands will result in the matcher checking that the number of operands
+ falls within that range.
+* ``GIVariadic<>`` operands can be used in C++ code within a rule, which will
+ result in the operand name being expanded to an iterator range containing all
+ the matched operands, similar to the one returned by ``MachineInstr::operands()``.
+
+.. code-block:: text
+
+ def build_vector_to_unmerge: GICombineRule <
+ (defs root:$root),
+ (match (G_BUILD_VECTOR $root, GIVariadic<>:$args),
+ [{ return checkBuildVectorToUnmerge(${args}); }]),
+ (apply (G_UNMERGE_VALUES $root, $args))
+ >;
+
+.. code-block:: text
+
+ // Will additionally check the number of operands is >= 3 and <= 5.
+ // ($root is one operand, then 2 to 4 variadic operands).
+ def build_vector_to_unmerge: GICombineRule <
+ (defs root:$root),
+ (match (G_BUILD_VECTOR $root, GIVariadic<2, 4>:$two_to_four),
+ [{ return checkBuildVectorToUnmerge(${two_to_four}); }]),
+ (apply (G_UNMERGE_VALUES $root, $two_to_four))
+ >;
+
Builtin Operations
------------------
@@ -240,6 +291,8 @@ This a non-exhaustive list of known issues with MIR patterns at this time.
match. e.g. if a pattern needs to work on both i32 and i64, you either
need to leave it untyped and check the type in C++, or duplicate the
pattern.
+* ``GISpecialType`` operands are not allowed within a ``GICombinePatFrag``.
+* ``GIVariadic<>`` matched operands must each have a unique name.
GICombineRule
-------------
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
index cc2dd2f4e489c..d7b9c96f1c01e 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
@@ -133,6 +133,12 @@ enum {
/// - Ops(ULEB128) - Expected number of operands
GIM_CheckNumOperands,
+ /// Check the instruction has a number of operands <= or >= than given number.
+ /// - InsnID(ULEB128) - Instruction ID
+ /// - Ops(ULEB128) - Number of operands
+ GIM_CheckNumOperandsLE,
+ GIM_CheckNumOperandsGE,
+
/// Check an immediate predicate on the specified instruction
/// - InsnID(ULEB128) - Instruction ID
/// - Pred(2) - The predicate to test
@@ -294,12 +300,15 @@ enum {
/// Check the specified operands are identical.
/// The IgnoreCopies variant looks through COPY instructions before
/// comparing the operands.
+ /// The "All" variants check all operands starting from the index.
/// - InsnID(ULEB128) - Instruction ID
/// - OpIdx(ULEB128) - Operand index
/// - OtherInsnID(ULEB128) - Other instruction ID
/// - OtherOpIdx(ULEB128) - Other operand index
GIM_CheckIsSameOperand,
GIM_CheckIsSameOperandIgnoreCopies,
+ GIM_CheckAllSameOperand,
+ GIM_CheckAllSameOperandIgnoreCopies,
/// Check we can replace all uses of a register with another.
/// - OldInsnID(ULEB128)
@@ -362,6 +371,13 @@ enum {
/// GIR_Copy but with both New/OldInsnIDs omitted and defaulting to zero.
GIR_RootToRootCopy,
+ /// Copies all operand starting from OpIdx in OldInsnID into the new
+ /// instruction NewInsnID.
+ /// - NewInsnID(ULEB128) - Instruction ID to modify
+ /// - OldInsnID(ULEB128) - Instruction ID to copy from
+ /// - OpIdx(ULEB128) - The first operand to copy
+ GIR_CopyRemaining,
+
/// Copy an operand to the specified instruction or add a zero register if the
/// operand is a zero immediate.
/// - NewInsnID(ULEB128) - Instruction ID to modify
diff --git a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
index 90b4fe5518c87..5a5a750ac6b4a 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
@@ -309,6 +309,23 @@ bool GIMatchTableExecutor::executeMatchTable(
break;
}
+ case GIM_CheckNumOperandsGE:
+ case GIM_CheckNumOperandsLE: {
+ uint64_t InsnID = readULEB();
+ uint64_t Expected = readULEB();
+ const bool IsLE = (MatcherOpcode == GIM_CheckNumOperandsLE);
+ DEBUG_WITH_TYPE(TgtExecutor::getName(),
+ dbgs() << CurrentIdx << ": GIM_CheckNumOperands"
+ << (IsLE ? "LE" : "GE") << "(MIs[" << InsnID
+ << "], Expected=" << Expected << ")\n");
+ assert(State.MIs[InsnID] != nullptr && "Used insn before defined");
+ const unsigned NumOps = State.MIs[InsnID]->getNumOperands();
+ if (IsLE ? (NumOps <= Expected) : (NumOps >= Expected)) {
+ if (handleReject() == RejectAndGiveUp)
+ return false;
+ }
+ break;
+ }
case GIM_CheckNumOperands: {
uint64_t InsnID = readULEB();
uint64_t Expected = readULEB();
@@ -1081,6 +1098,22 @@ bool GIMatchTableExecutor::executeMatchTable(
break;
}
+ case GIR_CopyRemaining: {
+ uint64_t NewInsnID = readULEB();
+ uint64_t OldInsnID = readULEB();
+ uint64_t OpIdx = readULEB();
+ assert(OutMIs[NewInsnID] && "Attempted to add to undefined instruction");
+ MachineInstr &OldMI = *State.MIs[OldInsnID];
+ MachineInstrBuilder &NewMI = OutMIs[NewInsnID];
+ for (const auto &Op : drop_begin(OldMI.operands(), OpIdx))
+ NewMI.add(Op);
+ DEBUG_WITH_TYPE(TgtExecutor::getName(),
+ dbgs() << CurrentIdx << ": GIR_CopyRemaining(OutMIs["
+ << NewInsnID << "], MIs[" << OldInsnID
+ << "], /*start=*/" << OpIdx << ")\n");
+ break;
+ }
+
case GIR_CopyOrAddZeroReg: {
uint64_t NewInsnID = readULEB();
uint64_t OldInsnID = readULEB();
diff --git a/llvm/include/llvm/Target/GlobalISel/Combine.td b/llvm/include/llvm/Target/GlobalISel/Combine.td
index 3ef0636ebf1c7..5c5ad1f6077a6 100644
--- a/llvm/include/llvm/Target/GlobalISel/Combine.td
+++ b/llvm/include/llvm/Target/GlobalISel/Combine.td
@@ -128,6 +128,16 @@ class GITypeOf<string opName> : GISpecialType {
string OpName = opName;
}
+// The type of an operand that can match a variable amount of operands.
+// This type contains a minimum and maximum number of operands to match.
+// The minimum must be 1 or more, as we cannot have an operand representing
+// zero operands, and the max can be zero (which means "unlimited") or a value
+// greater than the minimum.
+class GIVariadic<int min = 1, int max = 0> : GISpecialType {
+ int MinArgs = min;
+ int MaxArgs = max;
+}
+
//===----------------------------------------------------------------------===//
// Pattern Builtins
//===----------------------------------------------------------------------===//
diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-variadics.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-variadics.td
index 86ae031caecb5..e3061e2f4d3e5 100644
--- a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-variadics.td
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-variadics.td
@@ -28,19 +28,31 @@ def InstTest3 : GICombineRule<
(match (G_UNMERGE_VALUES $a, $b, $c, $d)),
(apply [{ APPLY }])>;
+def VariadicTypeTestCxx : GICombineRule<
+ (defs root:$a),
+ (match (G_BUILD_VECTOR $a, GIVariadic<2, 4>:$b)),
+ (apply [{ ${b} }])>;
+
+def VariadicTypeTestReuse : GICombineRule<
+ (defs root:$a),
+ (match (G_BUILD_VECTOR $a, $c, GIVariadic<2, 4>:$b)),
+ (apply (G_MERGE_VALUES $a, $b, $c))>;
+
def MyCombiner: GICombiner<"GenMyCombiner", [
InstTest0,
InstTest1,
InstTest2,
- InstTest3
+ InstTest3,
+ VariadicTypeTestCxx,
+ VariadicTypeTestReuse
]>;
// CHECK: const uint8_t *GenMyCombiner::getMatchTable() const {
// CHECK-NEXT: constexpr static uint8_t MatchTable0[] = {
-// CHECK-NEXT: GIM_SwitchOpcode, /*MI*/0, /*[*/GIMT_Encode2([[#LOWER:]]), GIMT_Encode2([[#UPPER:]]), /*)*//*default:*//*Label 2*/ GIMT_Encode4([[#DEFAULT:]]),
+// CHECK-NEXT: GIM_SwitchOpcode, /*MI*/0, /*[*/GIMT_Encode2(70), GIMT_Encode2(74), /*)*//*default:*//*Label 2*/ GIMT_Encode4(127),
// CHECK-NEXT: /*TargetOpcode::G_UNMERGE_VALUES*//*Label 0*/ GIMT_Encode4(26), GIMT_Encode4(0), GIMT_Encode4(0),
// CHECK-NEXT: /*TargetOpcode::G_BUILD_VECTOR*//*Label 1*/ GIMT_Encode4(55),
-// CHECK-NEXT: // Label 0: @[[#%u, mul(UPPER-LOWER, 4) + 10]]
+// CHECK-NEXT: // Label 0: @26
// CHECK-NEXT: GIM_Try, /*On fail goto*//*Label 3*/ GIMT_Encode4(40), // Rule ID 2 //
// CHECK-NEXT: GIM_CheckSimplePredicate, GIMT_Encode2(GICXXPred_Simple_IsRule2Enabled),
// CHECK-NEXT: GIM_CheckNumOperands, /*MI*/0, /*Expected*/2,
@@ -77,7 +89,35 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
// CHECK-NEXT: // Combiner Rule #1: InstTest1
// CHECK-NEXT: GIR_DoneWithCustomAction, /*Fn*/GIMT_Encode2(GICXXCustomAction_GICombiner0),
// CHECK-NEXT: // Label 5: @69
-// CHECK-NEXT: GIM_Try, /*On fail goto*//*Label 6*/ GIMT_Encode4(83), // Rule ID 0 //
+// CHECK-NEXT: GIM_Try, /*On fail goto*//*Label 6*/ GIMT_Encode4(86), // Rule ID 4 //
+// CHECK-NEXT: GIM_CheckSimplePredicate, GIMT_Encode2(GICXXPred_Simple_IsRule4Enabled),
+// CHECK-NEXT: GIM_CheckNumOperandsGE, /*MI*/0, /*Expected*/3,
+// CHECK-NEXT: GIM_CheckNumOperandsLE, /*MI*/0, /*Expected*/5,
+// CHECK-NEXT: // MIs[0] a
+// CHECK-NEXT: // No operand predicates
+// CHECK-NEXT: // MIs[0] b
+// CHECK-NEXT: // No operand predicates
+// CHECK-NEXT: // Combiner Rule #4: VariadicTypeTestCxx
+// CHECK-NEXT: GIR_DoneWithCustomAction, /*Fn*/GIMT_Encode2(GICXXCustomAction_GICombiner1),
+// CHECK-NEXT: // Label 6: @86
+// CHECK-NEXT: GIM_Try, /*On fail goto*//*Label 7*/ GIMT_Encode4(112), // Rule ID 5 //
+// CHECK-NEXT: GIM_CheckSimplePredicate, GIMT_Encode2(GICXXPred_Simple_IsRule5Enabled),
+// CHECK-NEXT: GIM_CheckNumOperandsGE, /*MI*/0, /*Expected*/4,
+// CHECK-NEXT: GIM_CheckNumOperandsLE, /*MI*/0, /*Expected*/6,
+// CHECK-NEXT: // MIs[0] a
+// CHECK-NEXT: // No operand predicates
+// CHECK-NEXT: // MIs[0] c
+// CHECK-NEXT: // No operand predicates
+// CHECK-NEXT: // MIs[0] b
+// CHECK-NEXT: // No operand predicates
+// CHECK-NEXT: // Combiner Rule #5: VariadicTypeTestReuse
+// CHECK-NEXT: GIR_BuildRootMI, /*Opcode*/GIMT_Encode2(TargetOpcode::G_MERGE_VALUES),
+// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/0, // a
+// CHECK-NEXT: GIR_CopyRemaining, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/2, // b
+// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/1, // c
+// CHECK-NEXT: GIR_EraseRootFromParent_Done,
+// CHECK-NEXT: // Label 7: @112
+// CHECK-NEXT: GIM_Try, /*On fail goto*//*Label 8*/ GIMT_Encode4(126), // Rule ID 0 //
// CHECK-NEXT: GIM_CheckSimplePredicate, GIMT_Encode2(GICXXPred_Simple_IsRule0Enabled),
// CHECK-NEXT: GIM_CheckNumOperands, /*MI*/0, /*Expected*/4,
// CHECK-NEXT: // MIs[0] a
@@ -90,10 +130,10 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
// CHECK-NEXT: // No operand predicates
// CHECK-NEXT: // Combiner Rule #0: InstTest0
// CHECK-NEXT: GIR_DoneWithCustomAction, /*Fn*/GIMT_Encode2(GICXXCustomAction_GICombiner0),
-// CHECK-NEXT: // Label 6: @83
+// CHECK-NEXT: // Label 8: @126
// CHECK-NEXT: GIM_Reject,
-// CHECK-NEXT: // Label 2: @[[#%u, DEFAULT]]
+// CHECK-NEXT: // Label 2: @127
// CHECK-NEXT: GIM_Reject,
-// CHECK-NEXT: }; // Size: [[#%u, DEFAULT + 1]] bytes
+// CHECK-NEXT: }; // Size: 128 bytes
// CHECK-NEXT: return MatchTable0;
// CHECK-NEXT: }
diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/operand-types.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/operand-types.td
index 4769bed972401..9b5598661c8de 100644
--- a/llvm/test/TableGen/GlobalISelCombinerEmitter/operand-types.td
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/operand-types.td
@@ -104,8 +104,32 @@ def TypeOfProp : GICombineRule<
(apply (G_ANYEXT $x, GITypeOf<"$y">:$tmp),
(G_ANYEXT $tmp, $y))>;
+// CHECK: (CombineRule name:VariadicTypeTest id:3 root:a
+// CHECK-NEXT: (MatchPats
+// CHECK-NEXT: <match_root>__VariadicTypeTest_match_0:(CodeGenInstructionPattern G_UNMERGE_VALUES operands:[<def>$a, <def>$b, GIVariadic<1,0>:$z])
+// CHECK-NEXT: )
+// CHECK-NEXT: (ApplyPats
+// CHECK-NEXT: <apply_root>__VariadicTypeTest_apply_0:(CodeGenInstructionPattern G_UNMERGE_VALUES operands:[<def>$a, <def>$b, GIVariadic<1,0>:$z])
+// CHECK-NEXT: )
+// CHECK-NEXT: (OperandTable MatchPats
+// CHECK-NEXT: a -> __VariadicTypeTest_match_0
+// CHECK-NEXT: b -> __VariadicTypeTest_match_0
+// CHECK-NEXT: z -> <live-in>
+// CHECK-NEXT: )
+// CHECK-NEXT: (OperandTable ApplyPats
+// CHECK-NEXT: a -> __VariadicTypeTest_apply_0
+// CHECK-NEXT: b -> __VariadicTypeTest_apply_0
+// CHECK-NEXT: z -> <live-in>
+// CHECK-NEXT: )
+// CHECK-NEXT: )
+def VariadicTypeTest: GICombineRule<
+ (defs root:$a),
+ (match (G_UNMERGE_VALUES $a, $b, GIVariadic<>:$z)),
+ (apply (G_UNMERGE_VALUES $a, $b, $z))>;
+
def MyCombiner: GICombiner<"GenMyCombiner", [
InstTest0,
PatFragTest0,
- TypeOfProp
+ TypeOfProp,
+ VariadicTypeTest,
]>;
diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/typeof-errors.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/typeof-errors.td
index ee7b8f5f3a39c..900a4bb902f7c 100644
--- a/llvm/test/TableGen/GlobalISelCombinerEmitter/typeof-errors.td
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/typeof-errors.td
@@ -21,7 +21,7 @@ def UnknownOperand : GICombineRule<
(match (G_ZEXT $dst, $src)),
(apply (G_ANYEXT $dst, (GITypeOf<"$unknown"> 0)))>;
-// CHECK: :[[@LINE+2]]:{{[0-9]+}}: error: GISpecialType is not supported in 'match' patterns
+// CHECK: :[[@LINE+2]]:{{[0-9]+}}: error: GITypeOf is not supported in 'match' patterns
// CHECK: :[[@LINE+1]]:{{[0-9]+}}: note: operand 1 of '__UseInMatch_match_0' has type 'GITypeOf<$dst>'
def UseInMatch : GICombineRule<
(defs root:$dst),
@@ -41,7 +41,7 @@ def UseInPF: GICombineRule<
(match (PFWithTypeOF $dst)),
(apply (G_ANYEXT $dst, (i32 0)))>;
-// CHECK: :[[@LINE+2]]:{{[0-9]+}}: error: GISpecialType is not supported in 'match' patterns
+// CHECK: :[[@LINE+2]]:{{[0-9]+}}: error: GITypeOf is not supported in 'match' patterns
// CHECK: :[[@LINE+1]]:{{[0-9]+}}: note: operand 1 of '__InferredUseInMatch_match_0' has type 'GITypeOf<$dst>'
def InferredUseInMatch : GICombineRule<
(defs root:$dst),
@@ -63,6 +63,13 @@ def TypeOfApplyTmp : GICombineRule<
(apply (G_ANYEXT $dst, i32:$tmp),
(G_ANYEXT $tmp, (GITypeOf<"$tmp"> 0)))>;
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: type 'GITypeOf<$src>' is ill-formed: 'src' is a variadic pack operand
+def TypeOfVariadic : GICombineRule<
+ (defs root:$dst),
+ (match (G_ZEXT $dst, GIVariadic<>:$src)),
+ (apply (G_ANYEXT GITypeOf<"$src">:$tmp, $src),
+ (G_ANYEXT $dst, $tmp))>;
+
// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Failed to parse one or more rules
def MyCombiner: GICombiner<"GenMyCombiner", [
NoDollarSign,
@@ -71,5 +78,6 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
UseInPF,
InferredUseInMatch,
InferenceConflict,
- TypeOfApplyTmp
+ TypeOfApplyTmp,
+ TypeOfVariadic
]>;
diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/variadic-errors.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/variadic-errors.td
new file mode 100644
index 0000000000000..68929c09da8d2
--- /dev/null
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/variadic-errors.td
@@ -0,0 +1,75 @@
+// RUN: not llvm-tblgen -I %p/../../../include -gen-global-isel-combiner \
+// RUN: -combiners=MyCombiner %s 2>&1| \
+// RUN: FileCheck %s -implicit-check-not=error:
+
+include "llvm/Target/Target.td"
+include "llvm/Target/GlobalISel/Combine.td"
+
+def MyTargetISA : InstrInfo;
+def MyTarget : Target { let InstructionSet = MyTargetISA; }
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: 'G_BUILD_VECTOR': GIVariadic can only be used on the last operand
+def VariadicNotLastInList : GICombineRule<
+ (defs root:$dst),
+ (match (G_BUILD_VECTOR $dst, $a, GIVariadic<>:$b, $c)),
+ (apply (G_ANYEXT $dst, $a))>;
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: 'G_IMPLICIT_DEF': GIVariadic cannot be used on defs
+def VariadicAsDef : GICombineRule<
+ (defs root:$dst),
+ (match (G_IMPLICIT_DEF GIVariadic<1>:$dst)),
+ (apply [{ APPLY }])>;
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: conflicting types for operand 'args': 'GIVariadic<2,4>' vs 'GIVariadic<3,6>'
+def ConflictingInference : GICombineRule<
+ (defs root:$dst),
+ (match (G_BUILD_VECTOR $dst, GIVariadic<2, 4>:$args)),
+ (apply (G_MERGE_VALUES $dst, GIVariadic<3, 6>:$args))>;
+
+// CHECK: :[[@LINE+2]]:{{[0-9]+}}: error: cannot parse operand type: minimum number of arguments must be greater than zero in GIVariadic
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Failed to parse pattern: '(G_BUILD_VECTOR ?:$dst, anonymous_8021:$a)'
+def InvalidBounds0 : GICombineRule<
+ (defs root:$dst),
+ (match (G_BUILD_VECTOR $dst, GIVariadic<0>:$a)),
+ (apply [{ APPLY }])>;
+
+// CHECK: :[[@LINE+2]]:{{[0-9]+}}: error: cannot parse operand type: maximum number of arguments (1) must be zero, or greater than the minimum number of arguments (1) in GIVariadic
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Failed to parse pattern: '(G_BUILD_VECTOR ?:$dst, anonymous_8022:$a)'
+def InvalidBounds1 : GICombineRule<
+ (defs root:$dst),
+ (match (G_BUILD_VECTOR $dst, GIVariadic<1,1>:$a)),
+ (apply [{ APPLY }])>;
+
+// CHECK: :[[@LINE+2]]:{{[0-9]+}}: error: each instance of a GIVariadic operand must have a unique name within the match patterns
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: note: 'c' is used multiple times
+def VariadicTypeTestEqOp : GICombineRule<
+ (defs root:$a),
+ (match (G_MERGE_VALUES $b, $c),
+ (G_BUILD_VECTOR $a, $b, GIVariadic<2, 4>:$c)),
+ (apply (G_MERGE_VALUES $a, $c))>;
+
+// TODO: We could support this if needed
+
+// CHECK: :[[@LINE+3]]:{{[0-9]+}}: error: GISpecialType is not supported in GICombinePatFrag
+// CHECK: :[[@LINE+2]]:{{[0-9]+}}: note: operand 1 of '__PFWithVariadic_alt0_pattern_0' has type 'GIVariadic<1,0
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Could not parse GICombinePatFrag 'PFWithVariadic'
+def PFWithVariadic: GICombinePatFrag<
+ (outs $dst), (ins),
+ [(pattern (G_ANYEXT $dst, GIVariadic<>:$b))]>;
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Failed to parse pattern: '(PFWithVariadic ?:$dst)'
+def UseInPF: GICombineRule<
+ (defs root:$dst),
+ (match (PFWithVariadic $dst)),
+ (apply (G_ANYEXT $dst, (i32 0)))>;
+
+// CHECK: :[[@LINE+1]]:{{[0-9]+}}: error: Failed to parse one or more rules
+def MyCombiner: GICombiner<"GenMyCombiner", [
+ VariadicNotLastInList,
+ VariadicAsDef,
+ ConflictingInference,
+ InvalidBounds0,
+ InvalidBounds1,
+ VariadicTypeTestEqOp,
+ UseInPF
+]>;
diff --git a/llvm/utils/TableGen/Common/GlobalISel/CodeExp...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
arsenm
reviewed
Jul 25, 2024
Comment on lines
+181
to
+182
| (match (G_BUILD_VECTOR $root, GIVariadic<>:$args), | ||
| [{ return checkBuildVectorToUnmerge(${args}); }]), |
There was a problem hiding this comment.
Include an example of unpacking the first N args? Like
G_BUILD_VECTOR $root, $srca, $srcb, ...:$remainder
There was a problem hiding this comment.
That's not yet implemented for MIR patterns. In C++ you can just convert the iterator_range to an ArrayRef I think.
I need some ideas on how to proceed
What kind of operations do we want?
- Only head/tail of the list, or do we need specific indexes?
- Do we need one element at a time, or multiple ?
Coming up with some example patterns would greatly help. I'm not sure how unpacking would be used in practice, because we don't have loops, so we can't do things like "for each element, emit this"
There was a problem hiding this comment.
Isn't that what's going in in VariadicTypeTestReuse?
There was a problem hiding this comment.
No, that just takes the N matched operands and puts them at a different position in the output instruction.
So $b can represent 2 to 4 operands, and the output instruction has $a, $b (so 2 to 4 operands being put here), and $c
It's a test to check that in apply patterns, a matched variadic can go anywhere, but in a match pattern it can only go at the end
Note that this whole thing is very much a prototype implementation, I put it up for review to discuss things like this and refine it
There was a problem hiding this comment.
Could you spell out the signature of checkBuildVectorToUnmerge? In particular, I am interested in the argument type.
There was a problem hiding this comment.
I thought ArrayRef<MachineOperand> would work out of the box but to my surprise, ArrayRef did not have a constructor from iterator_range. I tried adding one, but after 10 minutes of fighting templates (because we'd need one for both const and non-const ranges, and it apparently doesn't work), I gave up and made a helper function to do the conversion.
So now it's really just a ArrayRef<MachineOperand>, created through GIMatchTableExecutor::getVariadicOperands.
|
Can I write: May not compile. |
The variadic operand must have a name, other than that, yes it'll work. |
|
|
Does it work with |
tschuett
reviewed
Jul 25, 2024
| @@ -1405,20 +1415,28 @@ class InstructionOpcodeMatcher : public InstructionPredicateMatcher { | |||
| }; | |||
|
|
|||
| class InstructionNumOperandsMatcher final : public InstructionPredicateMatcher { | |||
| public: | |||
| enum CheckKind { CK_Eq, CK_LE, CK_GE }; | |||
What do you mean? |
Thanks. |
tschuett
reviewed
Jul 26, 2024
| @@ -1726,9 +1738,10 @@ OperandMatcher &InstructionMatcher::addPhysRegInput(Record *Reg, unsigned OpIdx, | |||
|
|
|||
| void InstructionMatcher::emitPredicateOpcodes(MatchTable &Table, | |||
| RuleMatcher &Rule) { | |||
| if (NumOperandsCheck) | |||
| InstructionNumOperandsMatcher(InsnVarID, getNumOperands()) | |||
| if (canAddNumOperandsCheck()) { | |||
There was a problem hiding this comment.
Are the curly braces necessary for semantics?
There was a problem hiding this comment.
I think the coding style recommends to use curly braces if the body of the if is more than 1 line, even if it's just a single statement that is more than 80 cols.
There was a problem hiding this comment.
There was a problem hiding this comment.
"Similarly, braces should be used when a single-statement body is complex enough that it becomes difficult to see where the block containing the following statement began. An if/else chain or a loop is considered a single statement for this rule, and this rule applies recursively."
I will always vote in favor of braces
There was a problem hiding this comment.
I think it's just a grey area. I'm the primary contributor to these files and I always use curly braces when the if's body spans multiple lines, and I think consistency takes priority.
tschuett
reviewed
Jul 26, 2024
| NumOperandsCheck = false; | ||
| // FIXME: Is this even needed still? Why the isVariadicNumOperands check? | ||
| if (canAddNumOperandsCheck() && OpcMatcher.isVariadicNumOperands() && | ||
| getNumOperandMatchers() != 0) { |
There was a problem hiding this comment.
Are the curly braces necessary for semantics?
arsenm
reviewed
Jul 26, 2024
| @@ -713,6 +731,12 @@ class GIMatchTableExecutor { | |||
| return Ret; | |||
| } | |||
|
|
|||
| static ArrayRef<MachineOperand> getVariadicOperands(const MachineInstr &MI, | |||
There was a problem hiding this comment.
Rename to something more generic, given that the number of operands to skip. Alternatively infer from the instruction properties where the variadic arguments begin
There was a problem hiding this comment.
Will do, this can probably go into a separate file as well with other Combiner utils.
The direction I took with GIVariadic is that it's just a "variable operand pack", so 1 or more operands bundled together. It's not tied to the nature of the instruction itself.
You could write G_MUL $x, GIVariadic<>:$lhsAndRhs and that would also work.
Writing that now I realize that maybe this should be called something else, maybe something like a operand "bundle" or "pack". Or I should revert that decision and make it all about variadic instructions?
I like the idea of making them neutral because it gives more room to breathe for future feature, for instance if we have some kind of templated pattern for multiple opcodes each with a different number of inputs, we could use a GIVariadic to model the operands.
|
For awareness: This is pretty much a WIP, it's complete as-is and could land - hence why no WIP tag - but suggestions are highly welcome. If you feel like this isn't usable without some limitations being lifted (such as having variadic defs), or some feature added (like extracting individual operands in the pack), let me know and give me your thoughts/syntax ideas/etc. I can make a forum post if the discussion really picks up too |
|
This is awesome. |
|
|
|
defs will probably need a different syntax. |
I'm really not convinced this count as a misuse, but I also struggle to imagine a use case where GIVariadic could be used outside a variadic instruction. I was thinking having templated pattern where the instructions have different number of opcodes but it's kind
For defs we could just have things like |
|
This is already great. If somebody needs variadic defs, he can open a ticket. |
|
A future version: would also be great. |
|
lgtm |
|
There was no formal GitHub acceptance! |
|
I thought it was good given you and @arsenm said it looked good. Do you need me to revert and reopen a review? |
|
Let's leave it as is, but we both said lgtm without actual GitHub style acceptance. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/3675 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/160/builds/2605 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/180/builds/2604 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/3838 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/4804 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/2681 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/2661 Here is the relevant piece of the build log for the reference: |
|
Some patterns appear to have been renumbered after rebasing, causing the error. I fixed it in ab33c3d |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/2680 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/2737 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/3938 Here is the relevant piece of the build log for the reference: |
dyung
reviewed
Aug 1, 2024
| // CHECK: const uint8_t *GenMyCombiner::getMatchTable() const { | ||
| // CHECK-NEXT: constexpr static uint8_t MatchTable0[] = { | ||
| // CHECK-NEXT: GIM_SwitchOpcode, /*MI*/0, /*[*/GIMT_Encode2([[#LOWER:]]), GIMT_Encode2([[#UPPER:]]), /*)*//*default:*//*Label 2*/ GIMT_Encode4([[#DEFAULT:]]), | ||
| // CHECK-NEXT: GIM_SwitchOpcode, /*MI*/0, /*[*/GIMT_Encode2(70), GIMT_Encode2(74), /*)*//*default:*//*Label 2*/ GIMT_Encode4(127), |
There was a problem hiding this comment.
This change from using a "regex" to hard coding the numbers is causing the test to fail in our downstream repo. Is there a reason why we cannot update the previous test variables instead of just hard coding values in here for this test?
There was a problem hiding this comment.
There is no reason, I just forgot that those tests were changed to use regexes and I generally update them by copy-pasting the output of tblgen. I will make a patch to fix it.
tschuett
added a commit
to tschuett/llvm-project
that referenced
this pull request
Aug 10, 2024
tschuett
added a commit
to tschuett/llvm-project
that referenced
this pull request
Aug 16, 2024
tschuett
added a commit
to tschuett/llvm-project
that referenced
this pull request
Aug 17, 2024
tschuett
added a commit
to tschuett/llvm-project
that referenced
this pull request
Aug 17, 2024
tschuett
added a commit
that referenced
this pull request
Aug 21, 2024
cjdb
pushed a commit
to cjdb/llvm-project
that referenced
this pull request
Aug 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Allow for matching & rewriting a variable number of arguments in an instructions.
Solves #87459