Skip to content

[GISel] Erase the root instruction after emitting all its potential uses #77494

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 13, 2024
Merged

[GISel] Erase the root instruction after emitting all its potential uses #77494

merged 1 commit into from
Jan 13, 2024

Conversation

s-barannikov
Copy link
Contributor

This tries to fix a bug by resolving a few FIXMEs. The bug is that EraseInstAction is emitted after emitting the first BuildMIAction, which is too early because the erased instruction may still be used by subsequent BuildMIActions (in particular, by CopyRenderer).

An example of the bug (from match-table-operand-types.td):

def InstTest0 : GICombineRule<
  (defs root:$a),
  (match  (G_MUL i32:$x, i32:$b, i32:$c),
          (G_MUL $a, i32:$b, i32:$x)),
  (apply  (G_ADD i64:$tmp, $b, i32:$c),
          (G_ADD i8:$a, $b, i64:$tmp))>;

GIR_EraseFromParent, /*InsnID*/0,
GIR_BuildMI, /*InsnID*/1, /*Opcode*/GIMT_Encode2(TargetOpcode::G_ADD),
GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/0, // a
GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/1, // b
GIR_AddSimpleTempRegister, /*InsnID*/1, /*TempRegID*/0,

Here, the root instruction is destroyed before copying its operands ('a' and 'b') to the new instruction.

The solution is to emit EraseInstAction for the root instruction as the last action in the emission pipeline.

@s-barannikov
[GISel] Erase the root instruction after emitting all its potential uses
b5611aa 
This tries to fix a bug by resolving a few FIXMEs.
The bug is that `EraseInstAction` is emitted after emitting the _first_
`BuildMIAction`, which is too early because the erased instruction may
still be used by subsequent `BuildMIAction`s (in particular, by
`CopyRenderer`).

An example of the bug (from `match-table-operand-types.td`):
```
def InstTest0 : GICombineRule<
  (defs root:$a),
  (match  (G_MUL i32:$x, i32:$b, i32:$c),
          (G_MUL $a, i32:$b, i32:$x)),
  (apply  (G_ADD i64:$tmp, $b, i32:$c),
          (G_ADD i8:$a, $b, i64:$tmp))>;

GIR_EraseFromParent, /*InsnID*/0,
GIR_BuildMI, /*InsnID*/1, /*Opcode*/GIMT_Encode2(TargetOpcode::G_ADD),
GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/0, // a
GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/1, // b
GIR_AddSimpleTempRegister, /*InsnID*/1, /*TempRegID*/0,
```

Here, the root instruction is destroyed before copying its operands
('a' and 'b') to the new instruction.

The solution is to emit `EraseInstAction` for the root instruction
as the last action in the emission pipeline.
@llvmbot
Copy link
Member

llvmbot commented Jan 9, 2024

@llvm/pr-subscribers-llvm-globalisel

Author: Sergei Barannikov (s-barannikov)

Changes

This tries to fix a bug by resolving a few FIXMEs. The bug is that EraseInstAction is emitted after emitting the first BuildMIAction, which is too early because the erased instruction may still be used by subsequent BuildMIActions (in particular, by CopyRenderer).

An example of the bug (from match-table-operand-types.td):

def InstTest0 : GICombineRule&lt;
  (defs root:$a),
  (match  (G_MUL i32:$x, i32:$b, i32:$c),
          (G_MUL $a, i32:$b, i32:$x)),
  (apply  (G_ADD i64:$tmp, $b, i32:$c),
          (G_ADD i8:$a, $b, i64:$tmp))&gt;;

GIR_EraseFromParent, /*InsnID*/0,
GIR_BuildMI, /*InsnID*/1, /*Opcode*/GIMT_Encode2(TargetOpcode::G_ADD),
GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/0, // a
GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/1, // b
GIR_AddSimpleTempRegister, /*InsnID*/1, /*TempRegID*/0,

Here, the root instruction is destroyed before copying its operands ('a' and 'b') to the new instruction.

The solution is to emit EraseInstAction for the root instruction as the last action in the emission pipeline.

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

22 Files Affected:

  • (modified) llvm/test/TableGen/DefaultOpsGlobalISel.td (+9-9)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/builtins/match-table-replacerreg.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-operand-types.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-permutations.td (+8-8)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-typeof.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-input-discard.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-multiple-output-discard.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-multiple-output.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-nested-subregs.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-output-discard.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitter-zero-reg.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitter.td (+21-21)
  • (modified) llvm/test/TableGen/GlobalISelEmitterMatchTableOptimizer.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitterMatchTableOptimizerSameOperand-invalid.td (+2-2)
  • (modified) llvm/test/TableGen/GlobalISelEmitterRegSequence.td (+1-1)
  • (modified) llvm/test/TableGen/GlobalISelEmitterSubreg.td (+8-8)
  • (modified) llvm/test/TableGen/HasNoUse.td (+1-1)
  • (modified) llvm/test/TableGen/gisel-physreg-input.td (+2-2)
  • (modified) llvm/utils/TableGen/GlobalISelCombinerEmitter.cpp (+5-6)
  • (modified) llvm/utils/TableGen/GlobalISelEmitter.cpp (+13-30)
  • (modified) llvm/utils/TableGen/GlobalISelMatchTable.cpp (+3-6)
diff --git a/llvm/test/TableGen/DefaultOpsGlobalISel.td b/llvm/test/TableGen/DefaultOpsGlobalISel.td
index 9fb95e852e4860..0c5aa0b912f549 100644
--- a/llvm/test/TableGen/DefaultOpsGlobalISel.td
+++ b/llvm/test/TableGen/DefaultOpsGlobalISel.td
@@ -53,8 +53,8 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(1), /*SubOperand*/1, // mods1
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(1), /*SubOperand*/0, // src1
 // CHECK-NEXT:       GIR_AddImm8, /*InsnID*/0, /*Imm*/0,
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       // GIR_Coverage, 3,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 0: @79
@@ -73,8 +73,8 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/0, // src0
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/2, // clamp
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/1, // omod
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       // GIR_Coverage, 2,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 1: @139
@@ -95,8 +95,8 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/1, // mods
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/0, // src
 // CHECK-NEXT:       GIR_AddImm8, /*InsnID*/0, /*Imm*/0,
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       // GIR_Coverage, 8,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 2: @207
@@ -115,8 +115,8 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/0, // src0
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/1, // omod
 // CHECK-NEXT:       GIR_AddImm8, /*InsnID*/0, /*Imm*/0,
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       // GIR_Coverage, 5,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 3: @265
@@ -141,8 +141,8 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/0, // src0
 // CHECK-NEXT:       GIR_AddSimpleTempRegister, /*InsnID*/0, /*TempRegID*/0,
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/1, // clamp
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       // GIR_Coverage, 7,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 4: @345
@@ -160,8 +160,8 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK-NEXT:       GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/0, // src0
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/1, // clamp
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       // GIR_Coverage, 0,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 5: @400
@@ -180,8 +180,8 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/0, // src0
 // CHECK-NEXT:       GIR_AddImm8, /*InsnID*/0, /*Imm*/93,
 // CHECK-NEXT:       GIR_ComplexSubOperandRenderer, /*InsnID*/0, /*RendererID*/GIMT_Encode2(0), /*SubOperand*/1, // clamp
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       // GIR_Coverage, 6,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 6: @458
@@ -198,8 +198,8 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // 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_AddImm8, /*InsnID*/0, /*Imm*/0,
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       // GIR_Coverage, 1,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 7: @503
@@ -216,8 +216,8 @@ def clamp : OperandWithDefaultOps <i1, (ops (i1 0))>;
 // 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_AddImm8, /*InsnID*/0, /*Imm*/0,
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       // GIR_Coverage, 4,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 8: @548
diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/builtins/match-table-replacerreg.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/builtins/match-table-replacerreg.td
index 38ee0166b869d8..cf57a247bc797d 100644
--- a/llvm/test/TableGen/GlobalISelCombinerEmitter/builtins/match-table-replacerreg.td
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/builtins/match-table-replacerreg.td
@@ -54,8 +54,8 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:       GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // a
 // CHECK-NEXT:       GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/GIMT_Encode2(RegState::Define),
 // CHECK-NEXT:       GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/1, /*OpIdx*/2, // y
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_ReplaceRegWithTempReg, /*OldInsnID*/0, /*OldOpIdx*/1, /*TempRegID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 3: @525
 // CHECK-NEXT:     GIM_Reject,
diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-operand-types.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-operand-types.td
index a441b0e01ebec9..0fb63bce1d6a6e 100644
--- a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-operand-types.td
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-operand-types.td
@@ -39,11 +39,11 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:       GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/GIMT_Encode2(RegState::Define),
 // CHECK-NEXT:       GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // b
 // CHECK-NEXT:       GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/1, /*OpIdx*/2, // c
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_BuildMI, /*InsnID*/1, /*Opcode*/GIMT_Encode2(TargetOpcode::G_ADD),
 // CHECK-NEXT:       GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/0, // a
 // CHECK-NEXT:       GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/1, // b
 // CHECK-NEXT:       GIR_AddSimpleTempRegister, /*InsnID*/1, /*TempRegID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 0: @81
 // CHECK-NEXT:     GIM_Reject,
diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-permutations.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-permutations.td
index 2b5cfb4f6de266..c38c4be9d54558 100644
--- a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-permutations.td
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-permutations.td
@@ -196,8 +196,8 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:         GIR_BuildMI, /*InsnID*/0, /*Opcode*/GIMT_Encode2(TargetOpcode::COPY),
 // CHECK-NEXT:         GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
 // CHECK-NEXT:         GIR_AddSimpleTempRegister, /*InsnID*/0, /*TempRegID*/0,
-// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_CustomAction, GIMT_Encode2(GICXXCustomAction_CombineApplyGICombiner0),
+// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_Done,
 // CHECK-NEXT:       // Label 1: @99
 // CHECK-NEXT:       GIM_Try, /*On fail goto*//*Label 2*/ GIMT_Encode4(199), // Rule ID 6 //
@@ -239,8 +239,8 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:         GIR_BuildMI, /*InsnID*/0, /*Opcode*/GIMT_Encode2(TargetOpcode::COPY),
 // CHECK-NEXT:         GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
 // CHECK-NEXT:         GIR_AddSimpleTempRegister, /*InsnID*/0, /*TempRegID*/0,
-// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_CustomAction, GIMT_Encode2(GICXXCustomAction_CombineApplyGICombiner0),
+// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_Done,
 // CHECK-NEXT:       // Label 2: @199
 // CHECK-NEXT:       GIM_Try, /*On fail goto*//*Label 3*/ GIMT_Encode4(299), // Rule ID 5 //
@@ -282,8 +282,8 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:         GIR_BuildMI, /*InsnID*/0, /*Opcode*/GIMT_Encode2(TargetOpcode::COPY),
 // CHECK-NEXT:         GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
 // CHECK-NEXT:         GIR_AddSimpleTempRegister, /*InsnID*/0, /*TempRegID*/0,
-// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_CustomAction, GIMT_Encode2(GICXXCustomAction_CombineApplyGICombiner0),
+// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_Done,
 // CHECK-NEXT:       // Label 3: @299
 // CHECK-NEXT:       GIM_Try, /*On fail goto*//*Label 4*/ GIMT_Encode4(409), // Rule ID 4 //
@@ -329,8 +329,8 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:         GIR_BuildMI, /*InsnID*/0, /*Opcode*/GIMT_Encode2(TargetOpcode::COPY),
 // CHECK-NEXT:         GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
 // CHECK-NEXT:         GIR_AddSimpleTempRegister, /*InsnID*/0, /*TempRegID*/0,
-// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_CustomAction, GIMT_Encode2(GICXXCustomAction_CombineApplyGICombiner0),
+// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_Done,
 // CHECK-NEXT:       // Label 4: @409
 // CHECK-NEXT:       GIM_Try, /*On fail goto*//*Label 5*/ GIMT_Encode4(509), // Rule ID 3 //
@@ -372,8 +372,8 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:         GIR_BuildMI, /*InsnID*/0, /*Opcode*/GIMT_Encode2(TargetOpcode::COPY),
 // CHECK-NEXT:         GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
 // CHECK-NEXT:         GIR_AddSimpleTempRegister, /*InsnID*/0, /*TempRegID*/0,
-// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_CustomAction, GIMT_Encode2(GICXXCustomAction_CombineApplyGICombiner0),
+// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_Done,
 // CHECK-NEXT:       // Label 5: @509
 // CHECK-NEXT:       GIM_Try, /*On fail goto*//*Label 6*/ GIMT_Encode4(619), // Rule ID 2 //
@@ -419,8 +419,8 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:         GIR_BuildMI, /*InsnID*/0, /*Opcode*/GIMT_Encode2(TargetOpcode::COPY),
 // CHECK-NEXT:         GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
 // CHECK-NEXT:         GIR_AddSimpleTempRegister, /*InsnID*/0, /*TempRegID*/0,
-// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_CustomAction, GIMT_Encode2(GICXXCustomAction_CombineApplyGICombiner0),
+// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_Done,
 // CHECK-NEXT:       // Label 6: @619
 // CHECK-NEXT:       GIM_Try, /*On fail goto*//*Label 7*/ GIMT_Encode4(729), // Rule ID 1 //
@@ -466,8 +466,8 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:         GIR_BuildMI, /*InsnID*/0, /*Opcode*/GIMT_Encode2(TargetOpcode::COPY),
 // CHECK-NEXT:         GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
 // CHECK-NEXT:         GIR_AddSimpleTempRegister, /*InsnID*/0, /*TempRegID*/0,
-// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_CustomAction, GIMT_Encode2(GICXXCustomAction_CombineApplyGICombiner0),
+// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_Done,
 // CHECK-NEXT:       // Label 7: @729
 // CHECK-NEXT:       GIM_Try, /*On fail goto*//*Label 8*/ GIMT_Encode4(849), // Rule ID 0 //
@@ -517,8 +517,8 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:         GIR_BuildMI, /*InsnID*/0, /*Opcode*/GIMT_Encode2(TargetOpcode::COPY),
 // CHECK-NEXT:         GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // dst
 // CHECK-NEXT:         GIR_AddSimpleTempRegister, /*InsnID*/0, /*TempRegID*/0,
-// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_CustomAction, GIMT_Encode2(GICXXCustomAction_CombineApplyGICombiner0),
+// CHECK-NEXT:         GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:         GIR_Done,
 // CHECK-NEXT:       // Label 8: @849
 // CHECK-NEXT:       GIM_Reject,
diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-typeof.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-typeof.td
index d8921df638fb01..ca653674d9c256 100644
--- a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-typeof.td
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table-typeof.td
@@ -32,11 +32,11 @@ def Test0 : GICombineRule<
 // CHECK-NEXT:       GIR_BuildMI, /*InsnID*/0, /*Opcode*/GIMT_Encode2(TargetOpcode::G_CONSTANT),
 // CHECK-NEXT:       GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/GIMT_Encode2(RegState::Define),
 // CHECK-NEXT:       GIR_AddCImm, /*InsnID*/0, /*Type*/uint8_t(-2), /*Imm*/GIMT_Encode8(42),
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_BuildMI, /*InsnID*/1, /*Opcode*/GIMT_Encode2(TargetOpcode::G_SUB),
 // CHECK-NEXT:       GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/0, // dst
 // CHECK-NEXT:       GIR_AddSimpleTempRegister, /*InsnID*/1, /*TempRegID*/1,
 // CHECK-NEXT:       GIR_AddSimpleTempRegister, /*InsnID*/1, /*TempRegID*/0,
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 0: @77
 // CHECK-NEXT:     GIM_Reject,
diff --git a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td
index 61dbbac745fa0a..5ec44b5e08d855 100644
--- a/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td
+++ b/llvm/test/TableGen/GlobalISelCombinerEmitter/match-table.td
@@ -204,8 +204,8 @@ def MyCombiner: GICombiner<"GenMyCombiner", [
 // CHECK-NEXT:       GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/1, /*OpIdx*/1, // ext
 // CHECK-NEXT:       GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // ptr
 // CHECK-NEXT:       GIR_MergeMemOperands, /*InsnID*/0, /*NumInsns*/2, /*MergeInsnID's*/0, 1,
-// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_CustomAction, GIMT_Encode2(GICXXCustomAction_CombineApplyGICombiner2),
+// CHECK-NEXT:       GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT:       GIR_Done,
 // CHECK-NEXT:     // Label 10: @578
 // CHECK-NEXT:     GIM_Reject,
diff --git a/llvm/test/TableGen/GlobalISelEmitter-input-discard.td b/llvm/test/TableGen/GlobalISelEmitter-input-discard.td
index 7731cdefac8554..8d3c6cb180aea1 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-input-discard.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-input-discard.td
@@ -24,8 +24,8 @@ def FOO : I<(outs GPR32:$dst), (ins GPR32Op:$src0, GPR32Op:$src1), []>;
 // GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // GISEL-NEXT: GIR_AddSimpleTempRegister, /*InsnID*/0, /*TempRegID*/0,
 // GISEL-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/3, // src1
-// GISEL-NEXT: GIR_EraseFromParent, /*InsnID*/0,
 // GISEL-NEXT: GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// GISEL-NEXT: GIR_EraseFromParent, /*InsnID*/0,
 def : Pat <
   (int_tgt_foo (i32 srcvalue), i32:$src1),
   (FOO (IMPLICIT_DEF), GPR32:$src1)
diff --git a/llvm/test/TableGen/GlobalISelEmitter-multiple-output-discard.td b/llvm/test/TableGen/GlobalISelEmitter-multiple-output-discard.td
index f962a7dadf7436..70991ea3b69c07 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-multiple-output-discard.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-multiple-output-discard.td
@@ -38,5 +38,5 @@ def : Pat<(two_out GPR32:$val), (THREE_OUTS GPR32:$val)>;
 // CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/1, // DstI[out2]
 // CHECK-NEXT: GIR_AddTempRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/GIMT_Encode2(RegState::Define|RegState::Dead),
 // CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/2, // val
-// CHECK-NEXT: GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT: GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// 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 37ce7416fa5138..f75988dadc73b1 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-multiple-output.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-multiple-output.td
@@ -82,5 +82,5 @@ def : Pat<(two_in GPR32:$i1, GPR32:$i2), (TWO_INS GPR32:$i2, GPR32:$i1)>;
 // 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,
 // CHECK-NEXT: GIR_ConstrainSelectedInstOperands, /*InsnID*/0,
+// 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 437686a69b5eac..234b19a146c150 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-nested-subregs.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-nested-subregs.td
@@ -54,9 +54,9 @@ def A0  : RegisterClass<"MyTarget", [i32], 32, (add a0)>;
 // CHECK-NEXT: GIR_BuildMI, /*InsnID*/0, /*Opcode*/GIMT_Encode2(TargetOpcode::COPY),
 // CHECK-NEXT: GIR_Copy, /*NewInsnID*/0, /*OldInsnID*/0, /*OpIdx*/0, // DstI[dst]
 // CHECK-NEXT: GIR_AddTempSubRegister, /*InsnID*/0, /*TempRegID*/0, /*TempRegFlags*/GIMT_Encode2(0), GIMT_Encode2(MyTarget::lo16),
-// CHECK-NEXT: GIR_EraseFromParent, /*InsnID*/0,
 // CHECK-NEXT: GIR_ConstrainOperandRC, /*InsnID*/0, /*Op*/0, GIMT_Encode2(MyTarget::A0wRegClassID),
 // CHECK-NEXT: GIR_ConstrainOperandRC, /*InsnID*/0, /*Op*/1, GIMT_Encode2(MyTarget::A0RegClassID),
+// CHECK-NEXT: GIR_EraseFromParent, /*InsnID*/0,
 def : Pat<(i16 (anyext i8:$src)),
           (i16 (EXTRACT_SUBREG
                  (i32 (INSERT_SUBREG
diff --git a/llvm/test/TableGen/GlobalISelEmitter-output-discard.td b/llvm/test/TableGen/GlobalISelEmitter-output-discard.td
index 50a9ef8ddf5733..30f9c5f4755550 100644
--- a/llvm/test/TableGen/Global...
[truncated]

@s-barannikov s-barannikov merged commit 8e8c954 into llvm:main Jan 13, 2024
@s-barannikov s-barannikov deleted the gisel/erase-inst-action branch January 13, 2024 08:22
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
@s-barannikov @justinfargnoli
[GISel] Erase the root instruction after emitting all its potential u…
4e3010a 
…ses (llvm#77494)

This tries to fix a bug by resolving a few FIXMEs. The bug is that
`EraseInstAction` is emitted after emitting the _first_ `BuildMIAction`,
which is too early because the erased instruction may still be used by
subsequent `BuildMIAction`s (in particular, by `CopyRenderer`).

An example of the bug (from `match-table-operand-types.td`):
```
def InstTest0 : GICombineRule<
  (defs root:$a),
  (match  (G_MUL i32:$x, i32:$b, i32:$c),
          (G_MUL $a, i32:$b, i32:$x)),
  (apply  (G_ADD i64:$tmp, $b, i32:$c),
          (G_ADD i8:$a, $b, i64:$tmp))>;

GIR_EraseFromParent, /*InsnID*/0,
GIR_BuildMI, /*InsnID*/1, /*Opcode*/GIMT_Encode2(TargetOpcode::G_ADD),
GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/0, // a
GIR_Copy, /*NewInsnID*/1, /*OldInsnID*/0, /*OpIdx*/1, // b
GIR_AddSimpleTempRegister, /*InsnID*/1, /*TempRegID*/0,
```

Here, the root instruction is destroyed before copying its operands ('a'
and 'b') to the new instruction.

The solution is to emit `EraseInstAction` for the root instruction as
the last action in the emission pipeline.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arsenm arsenm arsenm approved these changes

@Pierre-vh Pierre-vh Awaiting requested review from Pierre-vh

Assignees
No one assigned
Labels
llvm:globalisel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@s-barannikov @llvmbot @arsenm