[TableGen][GISel] Fix importing frameindex node #120921
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
The support added in llvm#90475 was inherently broken. The pattern in the added test is incorrect, and even though TableGen doesn't complain, the code generated from it is invalid and fails verification with the message "Use not jointly dominated by defs.". There is no way to magically transform `frameindex` to `tframeindex` as it happens for some other leaf nodes. `frameindex` can only be selected by custom C++ code or by using an `SDNodeXForm`. This patch removes special handling of `tframeindex` so that the pattern becomes unimportable, and fixes the handling of G_FRAME_INDEX, which shouldn't have set the operand's name. It also fixes the type of the result of G_FRAME_INDEX in order to get the correct type check (GIM_CheckPointerToAny instead of GIM_CheckType with scalar LLT argument).
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-tablegen Author: Sergei Barannikov (s-barannikov) ChangesThe support added in #90475 was inherently broken. The pattern in the added test is incorrect, and even though TableGen doesn't complain, the code generated from it is invalid and fails verification with the message "Use not jointly dominated by defs.". There is no way to magically transform This patch removes special handling of It also fixes the type of the result of G_FRAME_INDEX in order to get the correct type check (GIM_CheckPointerToAny instead of GIM_CheckType with scalar LLT argument). Full diff: https://github.com/llvm/llvm-project/pull/120921.diff 3 Files Affected:
diff --git a/llvm/include/llvm/Target/GenericOpcodes.td b/llvm/include/llvm/Target/GenericOpcodes.td
index c8f91cd0de5978..e134bab61bf63c 100644
--- a/llvm/include/llvm/Target/GenericOpcodes.td
+++ b/llvm/include/llvm/Target/GenericOpcodes.td
@@ -99,7 +99,7 @@ def G_PHI : GenericInstruction {
}
def G_FRAME_INDEX : GenericInstruction {
- let OutOperandList = (outs type0:$dst);
+ let OutOperandList = (outs ptype0:$dst);
let InOperandList = (ins unknown:$src2);
let hasSideEffects = false;
}
diff --git a/llvm/test/TableGen/GlobalISelEmitter-frameindex.td b/llvm/test/TableGen/GlobalISelEmitter-frameindex.td
index 232691465bb3bd..715e53ddbad08d 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-frameindex.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-frameindex.td
@@ -1,29 +1,67 @@
-// RUN: llvm-tblgen -gen-global-isel -optimize-match-table=false -I %p/../../include -I %p/Common %s -o - < %s | FileCheck %s
+// RUN: llvm-tblgen -gen-global-isel -optimize-match-table=false -I %p/../../include -I %p/Common %s | FileCheck %s
include "llvm/Target/Target.td"
include "GlobalISelEmitterCommon.td"
-def ADD : I<(outs GPR32:$dst), (ins GPR32:$src1, GPR32:$src2), []>;
-
-//===- Test a simple pattern with frame index operands. ----------------------===//
-//
-// CHECK: GIM_Try, /*On fail goto*//*Label [[LABEL_NUM:[0-9]+]]*/ GIMT_Encode4([[LABEL:[0-9]+]]),
-// CHECK-NEXT: GIM_CheckNumOperands, /*MI*/0, /*Expected*/2,
-// CHECK-NEXT: GIM_CheckOpcode, /*MI*/0, GIMT_Encode2(TargetOpcode::G_FRAME_INDEX),
-// CHECK-NEXT: // MIs[0] DstI[dst]
-// CHECK-NEXT: GIM_RootCheckType, /*Op*/0, /*Type*/GILLT_p0s32,
-// CHECK-NEXT: GIM_RootCheckRegBankForClass, /*Op*/0, /*RC*/GIMT_Encode2(MyTarget::GPR32RegClassID),
-// CHECK-NEXT: // MIs[0] fi
-// CHECK-NEXT: // No operand predicates
-// CHECK-NEXT: // (frameindex:{ *:[i32] }):$fi => (ADD:{ *:[i32] } (tframeindex:{ *:[i32] }):$fi, 0:{ *:[i32] })
-// CHECK-NEXT: GIR_BuildRootMI, /*Opcode*/GIMT_Encode2(MyTarget::ADD),
-// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/0, // DstI[dst]
-// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/1, // fi
-// CHECK-NEXT: GIR_AddImm8, /*InsnID*/0, /*Imm*/0,
-// CHECK-NEXT: GIR_RootConstrainSelectedInstOperands,
-// CHECK-NEXT: // GIR_Coverage, 0,
-// CHECK-NEXT: GIR_EraseRootFromParent_Done,
-// CHECK-NEXT: // Label [[LABEL_NUM]]: @[[LABEL]]
-// CHECK-NEXT: GIM_Reject,
-
-def : Pat<(p0 frameindex:$fi), (ADD tframeindex:$fi, 0)>;
+def ADDI : I<(outs GPR32:$dst), (ins GPR32:$src1, i32imm:$src2), []>;
+
+def to_tframeindex : SDNodeXForm<frameindex, [{}]>;
+
+def : GICustomOperandRenderer<"renderFrameIndex">,
+ GISDNodeXFormEquiv<to_tframeindex>;
+
+def : Pat<(frameindex:$fi), (ADDI (to_tframeindex $fi), 0)>;
+
+def : Pat<(ptradd frameindex:$fi, (i32 imm:$offset)),
+ (ADDI (to_tframeindex $fi), imm:$offset)>;
+
+// CHECK-LABEL: GIM_Try, /*On fail goto*//*Label 0*/ GIMT_Encode4(74), // Rule ID 1 //
+// CHECK-NEXT: GIM_CheckNumOperands, /*MI*/0, /*Expected*/3,
+// CHECK-NEXT: GIM_CheckOpcode, /*MI*/0, GIMT_Encode2(TargetOpcode::G_PTR_ADD),
+// CHECK-NEXT: // MIs[0] DstI[dst]
+// CHECK-NEXT: GIM_CheckPointerToAny, /*MI*/0, /*Op*/0, /*SizeInBits*/32,
+// CHECK-NEXT: GIM_RootCheckRegBankForClass, /*Op*/0, /*RC*/GIMT_Encode2(MyTarget::GPR32RegClassID),
+// CHECK-NEXT: // MIs[0] fi
+// CHECK-NEXT: GIM_CheckPointerToAny, /*MI*/0, /*Op*/1, /*SizeInBits*/32,
+// CHECK-NEXT: GIM_RecordInsn, /*DefineMI*/1, /*MI*/0, /*OpIdx*/1, // MIs[1]
+// CHECK-NEXT: GIM_CheckNumOperands, /*MI*/1, /*Expected*/2,
+// CHECK-NEXT: GIM_CheckOpcode, /*MI*/1, GIMT_Encode2(TargetOpcode::G_FRAME_INDEX),
+// CHECK-NEXT: // MIs[1] Operand 0
+// CHECK-NEXT: GIM_CheckPointerToAny, /*MI*/1, /*Op*/0, /*SizeInBits*/32,
+// CHECK-NEXT: // MIs[1] Operand 1
+// CHECK-NEXT: // No operand predicates
+// CHECK-NEXT: // MIs[0] offset
+// CHECK-NEXT: GIM_RootCheckType, /*Op*/2, /*Type*/GILLT_s32,
+// CHECK-NEXT: GIM_RecordInsn, /*DefineMI*/2, /*MI*/0, /*OpIdx*/2, // MIs[2]
+// CHECK-NEXT: GIM_CheckNumOperands, /*MI*/2, /*Expected*/2,
+// CHECK-NEXT: GIM_CheckOpcode, /*MI*/2, GIMT_Encode2(TargetOpcode::G_CONSTANT),
+// CHECK-NEXT: // MIs[2] Operand 0
+// CHECK-NEXT: GIM_CheckType, /*MI*/2, /*Op*/0, /*Type*/GILLT_s32,
+// CHECK-NEXT: // MIs[2] Operand 1
+// CHECK-NEXT: // No operand predicates
+// CHECK-NEXT: GIM_CheckIsSafeToFold, /*NumInsns*/2,
+// CHECK-NEXT: // (ptradd:{ *:[i32] } (frameindex:{ *:[i32] }):$fi, (imm:{ *:[i32] }):$offset) => (ADDI:{ *:[i32] } (to_tframeindex:{ *:[i32] } ?:{ *:[i32] }:$fi), (imm:{ *:[i32] }):$offset)
+// CHECK-NEXT: GIR_BuildRootMI, /*Opcode*/GIMT_Encode2(MyTarget::ADDI),
+// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/0, // DstI[dst]
+// CHECK-NEXT: GIR_CustomRenderer, /*InsnID*/0, /*OldInsnID*/1, /*Renderer*/GIMT_Encode2(GICR_renderFrameIndex), // fi
+// CHECK-NEXT: GIR_CopyConstantAsSImm, /*NewInsnID*/0, /*OldInsnID*/2, // offset
+// CHECK-NEXT: GIR_RootConstrainSelectedInstOperands,
+// CHECK-NEXT: // GIR_Coverage, 1,
+// CHECK-NEXT: GIR_EraseRootFromParent_Done,
+
+// CHECK-LABEL: GIM_Try, /*On fail goto*//*Label 1*/ GIMT_Encode4(109), // Rule ID 0 //
+// CHECK-NEXT: GIM_CheckNumOperands, /*MI*/0, /*Expected*/2,
+// CHECK-NEXT: GIM_CheckOpcode, /*MI*/0, GIMT_Encode2(TargetOpcode::G_FRAME_INDEX),
+// CHECK-NEXT: // MIs[0] DstI[dst]
+// CHECK-NEXT: GIM_CheckPointerToAny, /*MI*/0, /*Op*/0, /*SizeInBits*/32,
+// CHECK-NEXT: GIM_RootCheckRegBankForClass, /*Op*/0, /*RC*/GIMT_Encode2(MyTarget::GPR32RegClassID),
+// CHECK-NEXT: // MIs[0] Operand 1
+// CHECK-NEXT: // No operand predicates
+// CHECK-NEXT: // (frameindex:{ *:[i32] }):$fi => (ADDI:{ *:[i32] } (to_tframeindex:{ *:[i32] } ?:{ *:[i32] }:$fi), 0:{ *:[i32] })
+// CHECK-NEXT: GIR_BuildRootMI, /*Opcode*/GIMT_Encode2(MyTarget::ADDI),
+// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/0, // DstI[dst]
+// CHECK-NEXT: GIR_CustomRenderer, /*InsnID*/0, /*OldInsnID*/0, /*Renderer*/GIMT_Encode2(GICR_renderFrameIndex), // fi
+// CHECK-NEXT: GIR_AddImm8, /*InsnID*/0, /*Imm*/0,
+// CHECK-NEXT: GIR_RootConstrainSelectedInstOperands,
+// CHECK-NEXT: // GIR_Coverage, 0,
+// CHECK-NEXT: GIR_EraseRootFromParent_Done,
diff --git a/llvm/utils/TableGen/GlobalISelEmitter.cpp b/llvm/utils/TableGen/GlobalISelEmitter.cpp
index c1a626fe299045..16ccab34d23577 100644
--- a/llvm/utils/TableGen/GlobalISelEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelEmitter.cpp
@@ -836,7 +836,8 @@ Expected<InstructionMatcher &> GlobalISelEmitter::createAndImportSelDAGMatcher(
assert(SrcGIOrNull &&
"Expected to have already found an equivalent Instruction");
if (SrcGIOrNull->TheDef->getName() == "G_CONSTANT" ||
- SrcGIOrNull->TheDef->getName() == "G_FCONSTANT") {
+ SrcGIOrNull->TheDef->getName() == "G_FCONSTANT" ||
+ SrcGIOrNull->TheDef->getName() == "G_FRAME_INDEX") {
// imm/fpimm still have operands but we don't need to do anything with it
// here since we don't support ImmLeaf predicates yet. However, we still
// need to note the hidden operand to get GIM_CheckNumOperands correct.
@@ -844,11 +845,6 @@ Expected<InstructionMatcher &> GlobalISelEmitter::createAndImportSelDAGMatcher(
return InsnMatcher;
}
- if (SrcGIOrNull->TheDef->getName() == "G_FRAME_INDEX") {
- InsnMatcher.addOperand(OpIdx++, Src.getName(), TempOpIdx);
- return InsnMatcher;
- }
-
// Special case because the operand order is changed from setcc. The
// predicate operand needs to be swapped from the last operand to the first
// source.
@@ -1248,10 +1244,6 @@ Expected<action_iterator> GlobalISelEmitter::importExplicitUseRenderer(
DstMIBuilder.addRenderer<CopyRenderer>(Dst.getName());
return InsertPt;
}
- if (Dst.getOperator()->getName() == "tframeindex") {
- DstMIBuilder.addRenderer<CopyRenderer>(Dst.getName());
- return InsertPt;
- }
if (Dst.getOperator()->getName() == "imm") {
DstMIBuilder.addRenderer<CopyConstantAsImmRenderer>(Dst.getName());
return InsertPt;
|
arsenm
reviewed
Dec 23, 2024
| @@ -1248,10 +1244,6 @@ Expected<action_iterator> GlobalISelEmitter::importExplicitUseRenderer( | |||
| DstMIBuilder.addRenderer<CopyRenderer>(Dst.getName()); | |||
| return InsertPt; | |||
| } | |||
| if (Dst.getOperator()->getName() == "tframeindex") { | |||
There was a problem hiding this comment.
This should be the same as the time case above?
There was a problem hiding this comment.
I'm not sure. Technically, yes, it would match the behavior of SelectionDAG.
On the other hand, there is no strict equivalent of frameindex / tframeindex in GISel. There is just one G_FRAME_INDEX, which behaves more like G_CONSTANT in that it is a separate instruction rather than just a flavor of machine operand. The same is true for G_GLOBAL_VALUE, G_CONSTANT_POOL, maybe some others.
Adding CopyRenderer is the default behavior that should be taken if the node doesn't require a special treatment. That is, the check above and the removed check are just whitelisting supported leaf-like nodes. So, if we want to allow this node, the check should be kept. If we want to make it clear that the node is unsupported, it should be removed. I have no strong preference.
There was a problem hiding this comment.
In principle we could have G_ pseudos that have a direct inline frame index, which would be the equivalent of TargetFrameIndex. I think we should treat timm/tfpimm/tframeindex consistently here, and pass them through as-is
There was a problem hiding this comment.
OK, considering there are other t* nodes, I'll just remove the check and add CopyRenderer unconditionally. There is no good reason to treat tframeindex differently from other t* nodes, and whitelisting them isn't a scalable approach.
There was a problem hiding this comment.
Nah, this needs some larger refactoring. Will do it in a separate PR.
I'll restore the check for now.
There was a problem hiding this comment.
There aren't that many of these, and they are kind of a special case. It's only the constant-like opcodes have target equivalents
s-barannikov
changed the title
arsenm
approved these changes
Dec 23, 2024
| @@ -836,19 +836,15 @@ Expected<InstructionMatcher &> GlobalISelEmitter::createAndImportSelDAGMatcher( | |||
| assert(SrcGIOrNull && | |||
| "Expected to have already found an equivalent Instruction"); | |||
| if (SrcGIOrNull->TheDef->getName() == "G_CONSTANT" || | |||
| SrcGIOrNull->TheDef->getName() == "G_FCONSTANT") { | |||
| SrcGIOrNull->TheDef->getName() == "G_FCONSTANT" || | |||
| SrcGIOrNull->TheDef->getName() == "G_FRAME_INDEX") { | |||
There was a problem hiding this comment.
G_GLOBAL_VALUE should be the same
There was a problem hiding this comment.
I'll add it in a future PR along with a test.
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/73/builds/10826 Here is the relevant piece of the build log for the reference |
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.
The existing test case is not representative. Even though TableGen
doesn't complain, the code generated from it is invalid and fails
verification with the message "Use not jointly dominated by defs.".
There is no way to magically transform
frameindextotframeindexas it happens for some other leaf nodes.
frameindexcan only beselected by custom C++ code or by using an
SDNodeXForm.This patch makes the test representative one and fixes the handling of
G_FRAME_INDEX, which shouldn't have set the operand's name.It also fixes the type of the result of
G_FRAME_INDEXin order to getthe correct type check (
GIM_CheckPointerToAnyinstead ofGIM_CheckTypewith scalar LLT argument).