Skip to content

[GISel][TableGen] Fix accidental operand name clashes in patterns #74492

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 10, 2023

Conversation

atrosinenko
Copy link
Contributor

When importing instruction selection patterns into GlobalISel, the operands matched in the "source" DAG are copied into corresponding operands of the "destination" DAG according to their names (such as Rd). If multiple operands in the source DAG share the same name, a GIM_CheckIsSameOperand predicate makes instruction selector check the corresponding operands for equality (at compiler run-time) as part of matching the source pattern.

The Def operands of the root node of the destination DAG are handled specially. The operands of the instruction corresponding to the root node are taken and GIM_CheckRegBankForClass predicates are tablegen-erated accordingly. If by coincidence the Def operand in question has the same name as one of the named operands in the pattern, a GIM_CheckIsSameOperand predicate is automatically added that is likely to prevent matching the source of otherwise applicable selection pattern at compiler run-time.

This patch mangles the Def operand names taken from the instruction corresponding to the root of the destination DAG (for example, "Rd" becomes "DstI[Rd]") preventing unexpected name clashes with pattern's named operands.

The patch consists of three sets of changes:

  • changes to the GlobalISelEmitter.cpp file are the actual fix
  • a test case is added to GlobalISelEmitter.td file as a regression test
  • everything else is the biggest and least interesting part - updates to the existing test cases: renames of the form Rd -> DstI[Rd] inside the inline comments in tablegen-erated code

@llvmbot
Copy link
Member

llvmbot commented Dec 5, 2023

@llvm/pr-subscribers-llvm-globalisel

Author: Anatoly Trosinenko (atrosinenko)

Changes

When importing instruction selection patterns into GlobalISel, the operands matched in the "source" DAG are copied into corresponding operands of the "destination" DAG according to their names (such as Rd). If multiple operands in the source DAG share the same name, a GIM_CheckIsSameOperand predicate makes instruction selector check the corresponding operands for equality (at compiler run-time) as part of matching the source pattern.

The Def operands of the root node of the destination DAG are handled specially. The operands of the instruction corresponding to the root node are taken and GIM_CheckRegBankForClass predicates are tablegen-erated accordingly. If by coincidence the Def operand in question has the same name as one of the named operands in the pattern, a GIM_CheckIsSameOperand predicate is automatically added that is likely to prevent matching the source of otherwise applicable selection pattern at compiler run-time.

This patch mangles the Def operand names taken from the instruction corresponding to the root of the destination DAG (for example, "Rd" becomes "DstI[Rd]") preventing unexpected name clashes with pattern's named operands.

The patch consists of three sets of changes:

  • changes to the GlobalISelEmitter.cpp file are the actual fix
  • a test case is added to GlobalISelEmitter.td file as a regression test
  • everything else is the biggest and least interesting part - updates to the existing test cases: renames of the form Rd -> DstI[Rd] inside the inline comments in tablegen-erated code

Patch is 73.03 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/74492.diff

17 Files Affected:

  • (modified) llvm/test/TableGen/ContextlessPredicates.td (+1-1)
  • (modified) llvm/test/TableGen/DefaultOpsGlobalISel.td (+9-9)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-input-discard.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-multiple-output-discard.td (+4-4)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-multiple-output.td (+6-6)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-nested-subregs.td (+2-2)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-output-discard.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-zero-reg.td (+2-2)
  • (modified) llvm/test/TableGen/GlobalISelEmitter.td (+78-49)
  • (modified) llvm/test/TableGen/GlobalISelEmitterCustomPredicate.td (+5-5)
  • (modified) llvm/test/TableGen/GlobalISelEmitterHwModes.td (+4-4)
  • (modified) llvm/test/TableGen/GlobalISelEmitterMatchTableOptimizer.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitterMatchTableOptimizerSameOperand-invalid.td (+2-2)
  • (modified) llvm/test/TableGen/GlobalISelEmitterRegSequence.td (+2-2)
  • (modified) llvm/test/TableGen/GlobalISelEmitterSubreg.td (+8-8)
  • (modified) llvm/test/TableGen/gisel-physreg-input.td (+4-4)
  • (modified) llvm/utils/TableGen/GlobalISelEmitter.cpp (+31-25)
diff --git a/llvm/test/TableGen/ContextlessPredicates.td b/llvm/test/TableGen/ContextlessPredicates.td
index 7f081e9a0ec00..0f4c4d0c450e1 100644
--- a/llvm/test/TableGen/ContextlessPredicates.td
+++ b/llvm/test/TableGen/ContextlessPredicates.td
@@ -26,7 +26,7 @@ def : Pat<(test_atomic_op_frag GPR32:$ptr, GPR32:$val) ,
 // CHECK_NOPT-NEXT:      GIM_CheckNumOperands, /*MI*/0, /*Expected*/3,
 // CHECK_NOPT-NEXT:      GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_ATOMICRMW_XCHG,
 // CHECK_NOPT-NEXT:      GIM_CheckMemorySizeEqualTo, /*MI*/0, /*MMO*/0, /*Size*/4,
-// CHECK_NOPT-NEXT:      // MIs[0] dst
+// CHECK_NOPT-NEXT:      // MIs[0] DstI[dst]
 // CHECK_NOPT-NEXT:      GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
 // CHECK_NOPT-NEXT:      GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
 // CHECK_NOPT-NEXT:      // MIs[0] ptr
diff --git a/llvm/test/TableGen/DefaultOpsGlobalISel.td b/llvm/test/TableGen/DefaultOpsGlobalISel.td
index 13ee2631ecb07..c997467c570ad 100644
--- a/llvm/test/TableGen/DefaultOpsGlobalISel.td
+++ b/llvm/test/TableGen/DefaultOpsGlobalISel.td
@@ -35,7 +35,7 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK: GIM_CheckComplexPattern, /*MI*/0, /*Op*/1, /*Renderer*/0, GICP_gi_SelectSrcMods,
 // CHECK: GIM_CheckComplexPattern, /*MI*/0, /*Op*/2, /*Renderer*/1, GICP_gi_SelectSrcMods,
 // CHECK: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::FMAX,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/1, // mods0
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/0, // src0
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/1, /*SubOperand*/1, // mods1
@@ -48,7 +48,7 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK: GIM_CheckComplexPattern, /*MI*/0, /*Op*/1, /*Renderer*/0, GICP_gi_SelectClampOMod,
 // CHECK: // (ffloor:{ *:[f32] } (SelectClampOMod:{ *:[f32] } f32:{ *:[f32] }:$src0, omod:{ *:[i32] }:$omod, i1:{ *:[i1] }:$clamp))  =>  (FLOMP:{ *:[f32] } f32:{ *:[f32] }:$src0, i1:{ *:[i1] }:$clamp, omod:{ *:[i32] }:$omod)
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::FLOMP,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/0, // src0
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/2, // clamp
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/1, // omod
@@ -56,7 +56,7 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 
 // CHECK: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_FCANONICALIZE,
 // CHECK: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::FMAX,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/1, // mods
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/0, // src
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/1, // mods
@@ -68,7 +68,7 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_FCOS,
 // CHECK: // (fcos:{ *:[f32] } (SelectOMod:{ *:[f32] } f32:{ *:[f32] }:$src0, i32:{ *:[i32] }:$omod))  =>  (FLAMP:{ *:[f32] } FPR32:{ *:[f32] }:$src0, omod:{ *:[i32] }:$omod)
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::FLAMP,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/0, // src0
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/1, // omod
 // CHECK-NEXT: GIR_AddImm, /*InsnID*/0, /*Imm*/0,
@@ -84,7 +84,7 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK-NEXT: GIR_AddImm, /*InsnID*/1, /*Imm*/0,
 // CHECK-NEXT: GIR_ConstrainSelectedInstOperands, /*InsnID*/1,
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::FEEPLE,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/0, // src0
 // CHECK-NEXT: GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/0,
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/1, // clamp
@@ -95,7 +95,7 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_FSIN,
 // CHECK: // (fsin:{ *:[f32] } (SelectClamp:{ *:[f32] } f32:{ *:[f32] }:$src0, i1:{ *:[i1] }:$clamp))  =>  (FFOO:{ *:[f32] } f32:{ *:[f32] }:$src0, i1:{ *:[i1] }:$clamp)
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::FFOO,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/0, // src0
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/1, // clamp
 // CHECK-NEXT: GIR_EraseFromParent, /*InsnID*/0,
@@ -104,7 +104,7 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_FSQRT,
 // CHECK: // (fsqrt:{ *:[f32] } (SelectClamp:{ *:[f32] } f32:{ *:[f32] }:$src0, i1:{ *:[i1] }:$clamp))  =>  (FLAMP:{ *:[f32] } FPR32:{ *:[f32] }:$src0, 93:{ *:[i32] }, clamp:{ *:[i1] }:$clamp)
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::FLAMP,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/0, // src0
 // CHECK-NEXT: GIR_AddImm, /*InsnID*/0, /*Imm*/93,
 // CHECK-NEXT: GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/1, // clamp
@@ -113,7 +113,7 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_INTRINSIC_ROUND,
 // CHECK: // (fround:{ *:[f32] } f32:{ *:[f32] }:$src0)  =>  (FBAR:{ *:[f32] } f32:{ *:[f32] }:$src0)
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::FBAR,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // src0
 // CHECK-NEXT: GIR_AddImm, /*InsnID*/0, /*Imm*/0,
 // CHECK-NEXT: GIR_EraseFromParent, /*InsnID*/0,
@@ -121,7 +121,7 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_INTRINSIC_TRUNC,
 // CHECK: // (ftrunc:{ *:[f32] } f32:{ *:[f32] }:$src0)  =>  (FFOO:{ *:[f32] } FPR32:{ *:[f32] }:$src0)
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::FFOO,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // src0
 // CHECK-NEXT: GIR_AddImm, /*InsnID*/0, /*Imm*/0,
 // CHECK-NEXT: GIR_EraseFromParent, /*InsnID*/0,
diff --git a/llvm/test/TableGen/GlobalISelEmitter-input-discard.td b/llvm/test/TableGen/GlobalISelEmitter-input-discard.td
index 6d0d1de5d06c0..68a9553a8b738 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-input-discard.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-input-discard.td
@@ -21,7 +21,7 @@ def FOO : I<(outs GPR32:$dst), (ins GPR32Op:$src0, GPR32Op:$src1), []>;
 // GISEL-NEXT: GIR_AddTempRegister, /*InsnID*/1, /*TempRegID*/0, /*TempRegFlags*/RegState::Define,
 // GISEL-NEXT: GIR_ConstrainSelectedInstOperands, /*InsnID*/1,
 // GISEL-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::FOO,
-// GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // GISEL-NEXT: GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/0,
 // GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/3, // src1
 // GISEL-NEXT: GIR_EraseFromParent, /*InsnID*/0,
diff --git a/llvm/test/TableGen/GlobalISelEmitter-multiple-output-discard.td b/llvm/test/TableGen/GlobalISelEmitter-multiple-output-discard.td
index 5f442b067d9cd..64cf31f3772ee 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-multiple-output-discard.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-multiple-output-discard.td
@@ -22,10 +22,10 @@ def : GINodeEquiv<G_TWO_OUT, two_out>;
 def : Pat<(two_out GPR32:$val), (THREE_OUTS GPR32:$val)>;
 
 // CHECK:      GIM_CheckOpcode, /*MI*/0, MyTarget::G_TWO_OUT,
-// CHECK-NEXT: // MIs[0] out1
+// CHECK-NEXT: // MIs[0] DstI[out1]
 // CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
 // CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
-// CHECK-NEXT: // MIs[0] out2
+// CHECK-NEXT: // MIs[0] DstI[out2]
 // CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/1, /*Type*/GILLT_s32,
 // CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
 // CHECK-NEXT: // MIs[0] val
@@ -34,8 +34,8 @@ def : Pat<(two_out GPR32:$val), (THREE_OUTS GPR32:$val)>;
 // CHECK-NEXT: // (two_out:{ *:[i32] }:{ *:[i32] } GPR32:{ *:[i32] }:$val)  =>  (THREE_OUTS:{ *:[i32] }:{ *:[i32] }:{ *:[i32] } GPR32:{ *:[i32] }:$val)
 // CHECK-NEXT: GIR_MakeTempReg, /*TempRegID*/0, /*TypeID*/GILLT_s32,
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::THREE_OUTS,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // out1
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // out2
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[out1]
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // DstI[out2]
 // CHECK-NEXT: GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/RegState::Define|RegState::Dead,
 // CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/2, // val
 // CHECK-NEXT: GIR_EraseFromParent, /*InsnID*/0,
diff --git a/llvm/test/TableGen/GlobalISelEmitter-multiple-output.td b/llvm/test/TableGen/GlobalISelEmitter-multiple-output.td
index c98ac73c61af7..85ac3ace0364c 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-multiple-output.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-multiple-output.td
@@ -30,10 +30,10 @@ def : Pat<(loadpost (p0 GPR32:$addr), (i32 GPR32:$off)),
 >;
 
 // CHECK:      GIM_CheckOpcode, /*MI*/0, MyTarget::G_POST_LOAD,
-// CHECK-NEXT: // MIs[0] val
+// CHECK-NEXT: // MIs[0] DstI[val]
 // CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
 // CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
-// CHECK-NEXT: // MIs[0] ptr_out
+// CHECK-NEXT: // MIs[0] DstI[ptr_out]
 // CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/1, /*Type*/GILLT_p0s32,
 // CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
 // CHECK-NEXT: // MIs[0] addr
@@ -64,10 +64,10 @@ def : GINodeEquiv<G_TWO_IN, two_in>;
 def : Pat<(two_in GPR32:$i1, GPR32:$i2), (TWO_INS GPR32:$i2, GPR32:$i1)>;
 
 // CHECK:      GIM_CheckOpcode, /*MI*/0, MyTarget::G_TWO_IN,
-// CHECK-NEXT: // MIs[0] out1
+// CHECK-NEXT: // MIs[0] DstI[out1]
 // CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
 // CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
-// CHECK-NEXT: // MIs[0] out2
+// CHECK-NEXT: // MIs[0] DstI[out2]
 // CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/1, /*Type*/GILLT_s32,
 // CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
 // CHECK-NEXT: // MIs[0] i1
@@ -78,8 +78,8 @@ def : Pat<(two_in GPR32:$i1, GPR32:$i2), (TWO_INS GPR32:$i2, GPR32:$i1)>;
 // CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/3, /*RC*/MyTarget::GPR32RegClassID,
 // CHECK-NEXT: // (two_in:{ *:[i32] }:{ *:[i32] } GPR32:{ *:[i32] }:$i1, GPR32:{ *:[i32] }:$i2)  =>  (TWO_INS:{ *:[i32] }:{ *:[i32] } GPR32:{ *:[i32] }:$i2, GPR32:{ *:[i32] }:$i1)
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::TWO_INS,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // out1
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // out2
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[out1]
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // DstI[out2]
 // CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/3, // i2
 // CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/2, // i1
 // CHECK-NEXT: GIR_EraseFromParent, /*InsnID*/0,
diff --git a/llvm/test/TableGen/GlobalISelEmitter-nested-subregs.td b/llvm/test/TableGen/GlobalISelEmitter-nested-subregs.td
index c48c82aa142ac..61f79bc04b27e 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-nested-subregs.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-nested-subregs.td
@@ -32,7 +32,7 @@ def A0  : RegisterClass<"MyTarget", [i32], 32, (add a0)>;
 
 // CHECK: GIM_CheckNumOperands, /*MI*/0, /*Expected*/2,
 // CHECK-NEXT: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_ANYEXT,
-// CHECK-NEXT: // MIs[0] dst
+// CHECK-NEXT: // MIs[0] DstI[dst]
 // CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s16,
 // CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::A0RegClassID,
 // CHECK-NEXT: // MIs[0] src
@@ -52,7 +52,7 @@ def A0  : RegisterClass<"MyTarget", [i32], 32, (add a0)>;
 // CHECK-NEXT: GIR_ConstrainOperandRC, /*InsnID*/1, /*Op*/1, MyTarget::A0RegClassID,
 // CHECK-NEXT: GIR_ConstrainOperandRC, /*InsnID*/1, /*Op*/2, MyTarget::A0bRegClassID,
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/TargetOpcode::COPY,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_AddTempSubRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/0, MyTarget::lo16,
 // CHECK-NEXT: GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT: GIR_ConstrainOperandRC, /*InsnID*/0, /*Op*/0, MyTarget::A0wRegClassID,
diff --git a/llvm/test/TableGen/GlobalISelEmitter-output-discard.td b/llvm/test/TableGen/GlobalISelEmitter-output-discard.td
index c755d8377e61d..1cfe49dee0970 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-output-discard.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-output-discard.td
@@ -15,7 +15,7 @@ def ADD_CO : I<(outs GPR32:$dst, GPR8:$flag),
 // GISEL-NEXT: // (add:{ *:[i32] } i32:{ *:[i32] }:$src0, i32:{ *:[i32] }:$src1)  =>  (ADD_CO:{ *:[i32] }:{ *:[i8] } GPR32:{ *:[i32] }:$src0, GPR32:{ *:[i32] }:$src1)
 // GISEL-NEXT: GIR_MakeTempReg, /*TempRegID*/0, /*TypeID*/GILLT_s8,
 // GISEL-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::ADD_CO,
-// GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // GISEL-NEXT: GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/RegState::Define|RegState::Dead,
 // GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // src0
 // GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/2, // src1
diff --git a/llvm/test/TableGen/GlobalISelEmitter-zero-reg.td b/llvm/test/TableGen/GlobalISelEmitter-zero-reg.td
index 374430bc427e8..63ad5d5cd03d9 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-zero-reg.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-zero-reg.td
@@ -24,7 +24,7 @@ def INST : PredI<(outs GPR32:$dst), (ins GPR32:$src), []>;
 // CHECK-NEXT: GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_LOAD,
 // CHECK-NEXT: GIM_CheckMemorySizeEqualToLLT, /*MI*/0, /*MMO*/0, /*OpIdx*/0,
 // CHECK-NEXT: GIM_CheckAtomicOrdering, /*MI*/0, /*Order*/(int64_t)AtomicOrdering::NotAtomic,
-// CHECK-NEXT: // MIs[0] dst
+// CHECK-NEXT: // MIs[0] DstI[dst]
 // CHECK-NEXT: GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
 // CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
 // CHECK-NEXT: // MIs[0] src
@@ -32,7 +32,7 @@ def INST : PredI<(outs GPR32:$dst), (ins GPR32:$src), []>;
 // CHECK-NEXT: GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
 // CHECK-NEXT: // (ld:{ *:[i32] } GPR32:{ *:[i32] }:$src)<<P:Predicate_unindexedload>><<P:Predicate_load>>  =>  (INST:{ *:[i32] } GPR32:{ *:[i32] }:$src)
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::INST,
-// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // src
 // CHECK-NEXT: GIR_AddRegister, /*InsnID*/0, MyTarget::NoRegister, /*AddRegisterRegFlags*/0,
 // CHECK-NEXT: GIR_MergeMemOperands, /*InsnID*/0, /*MergeInsnID's*/0, GIU_MergeMemOperands_EndOfList,
diff --git a/llvm/test/TableGen/GlobalISelEmitter.td b/llvm/test/TableGen/GlobalISelEmitter.td
index b7a81894f6442..55885cd17ac39 100644
--- a/llvm/test/TableGen/GlobalISelEmitter.td
+++ b/llvm/test/TableGen/GlobalISelEmitter.td
@@ -247,7 +247,7 @@ def HasC : Predicate<"Subtarget->hasC()"> { let RecomputePerFunction = 1; }
 // R19O-NEXT:    GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
 // R19N-NEXT:    GIM_CheckNumOperands, /*MI*/0, /*Expected*/4,
 // R19N-NEXT:    GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_SELECT,
-// R19N-NEXT:    // MIs[0] dst
+// R19N-NEXT:    // MIs[0] DstI[dst]
 // R19N-NEXT:    GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
 // R19N-NEXT:    GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
 // R19N-NEXT:    // MIs[0] src1
@@ -290,7 +290,7 @@ def HasC : Predicate<"Subtarget->hasC()"> { let RecomputePerFunction = 1; }
 // R19C-NEXT:    GIR_ComplexSubOperandRenderer, /*InsnID*/1, /*RendererID*/2, /*SubOperand*/1, // src5b
 // R19C-NEXT:    GIR_ConstrainSelectedInstOperands, /*InsnID*/1,
 // R19C-NEXT:    GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::INSN3,
-// R19C-NEXT:    GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// R19C-NEXT:    GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // R19C-NEXT:    GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // src1
 // R19C-NEXT:    GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/1, // src2b
 // R19C-NEXT:    GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/0, /*SubOperand*/0, // src2a
@@ -341,7 +341,7 @@ def : Pat<(select GPR32:$src1, (complex_rr GPR32:$src2a, GPR32:$src2b),
 // R21O-NEXT:    GIM_CheckRegBankForClass, /*MI*/0, /*Op*/1, /*RC*/MyTarget::GPR32RegClassID,
 // R21N-NEXT:    GIM_CheckNumOperands, /*MI*/0, /*Expected*/4,
 // R21N-NEXT:    GIM_CheckOpcode, /*MI*/0, TargetOpcode::G_SELECT,
-// R21N-NEXT:    // MIs[0] dst
+// R21N-NEXT:    // MIs[0] DstI[dst]
 // R21N-NEXT:    GIM_CheckType, /*MI*/0, /*Op*/0, /*Type*/GILLT_s32,
 // R21N-NEXT:    GIM_CheckRegBankForClass, /*MI*/0, /*Op*/0, /*RC*/MyTarget::GPR32RegClassID,
 // R21N-NEXT:    // MIs[0] src1
@@ -359,7 +359,7 @@ def : Pat<(select GPR32:$src1, (complex_rr GPR32:$src2a, GPR32:$src2b),
 // R21C-NEXT:    // (select:{ *:[i32] } GPR32:{ *:[i32] }:$src1, complex:{ *:[i32] }:$src2, complex:{ *:[i32] }:$src3)<<P:Predicate_frag>> => (INSN2:{ *:[i32] } GPR32:{ *:[i32] }:$src1, complex:{ *:[i32] }:$src3, complex:{ *:[i32] }:$src2)
 
 // R21C-NEXT:    GIR_BuildMI, /*InsnID*/0, /*Opcode*/MyTarget::INSN2,
-// R21C-NEXT:    GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
+// R21C-NEXT:    GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // R21C-NEXT:    GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // src1
 // R21C-NEXT: ...
[truncated]

@atrosinenko
Copy link
Contributor Author

An example of pattern that is not imported into GlobalISel correctly is

def : Pat<(int_ptrauth_blend GPR64:$Rd, GPR64:$Rn),
          (BFMXri GPR64:$Rd, GPR64:$Rn, 16, 15)>;

because BFMXri has (outs GPR64:$Rd).

Copy link
Member

@mshockwave mshockwave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Good catch!

When importing instruction selection patterns into GlobalISel, the
operands matched in the "source" DAG are copied into corresponding
operands of the "destination" DAG according to their names (such
as Rd). If multiple operands in the source DAG share the same name,
a GIM_CheckIsSameOperand predicate makes instruction selector check
the corresponding operands for equality (at compiler run-time) as part
of matching the source pattern.

The Def operands of the root node of the destination DAG are handled
specially. The operands of the instruction corresponding to the root
node are taken and GIM_CheckRegBankForClass predicates are
tablegen-erated accordingly. If by coincidence the Def operand in
question has the same name as one of the named operands in the pattern,
a GIM_CheckIsSameOperand predicate is automatically added that is likely
to prevent matching the source of otherwise applicable selection pattern
at compiler run-time.

This patch mangles the Def operand names taken from the instruction
corresponding to the root of the destination DAG (for example, "Rd"
becomes "DstI[Rd]") preventing unexpected name clashes with pattern's
named operands.

The patch consists of three sets of changes:
* changes to the GlobalISelEmitter.cpp file are the actual fix
* a test case is added to GlobalISelEmitter.td file as a regression test
* everything else is the biggest and least interesting part - updates to
  the existing test cases: renames of the form Rd -> DstI[Rd] inside the
  inline comments in tablegen-erated code
@atrosinenko atrosinenko force-pushed the tblgen-operand-name-clash branch from dd4c9c3 to a6a3651 Compare December 10, 2023 09:07
@atrosinenko
Copy link
Contributor Author

A few fixes after rebase:

  • a recent commit of @Pierre-vh added // Size: ... comments - just updated these two comments in the GlobalISelEmitter.td test file to contain the new values
  • fixed wording in the comment: "matcher" -> "predicate"

@atrosinenko atrosinenko merged commit 78623b0 into llvm:main Dec 10, 2023
@atrosinenko atrosinenko deleted the tblgen-operand-name-clash branch December 10, 2023 10:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants