[llvm][TableGen] Allow physical registers in patterns for GlobalISel emitter #113127
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
Most of the heavy lifting was already done in importExplicitDefRenderers, so I just added an operand for the physical register defs. The logic introduced in llvm#112673 was indeed subtly broken; it was only supposed to count the physical register defs present in the pattern rather than all physical register defs. Remove importImplicitDefRenderers since it was unimplemented anyway, and I don't see a way that it could be implemented satisfactorily. Also, the name was rather misleading; it should've been called something like importPhysRegDefRenderers
Member
|
@llvm/pr-subscribers-llvm-globalisel @llvm/pr-subscribers-tablegen Author: None (JL2210) ChangesMost of the heavy lifting was already done in The logic introduced in #112673 was indeed subtly broken; it was only supposed to count the physical register defs present in the pattern rather than all physical register defs. Remove Test courtesy of @s-barannikov Full diff: https://github.com/llvm/llvm-project/pull/113127.diff 3 Files Affected:
diff --git a/llvm/test/TableGen/Common/GlobalISelEmitterCommon.td b/llvm/test/TableGen/Common/GlobalISelEmitterCommon.td
index 8f11fee3751844..8f625811eb9b2e 100644
--- a/llvm/test/TableGen/Common/GlobalISelEmitterCommon.td
+++ b/llvm/test/TableGen/Common/GlobalISelEmitterCommon.td
@@ -7,7 +7,8 @@ class MyTargetGenericInstruction : GenericInstruction {
}
def R0 : Register<"r0"> { let Namespace = "MyTarget"; }
-def GPR32 : RegisterClass<"MyTarget", [i32], 32, (add R0)>;
+def R1 : Register<"r1"> { let Namespace = "MyTarget"; }
+def GPR32 : RegisterClass<"MyTarget", [i32], 32, (add R0, R1)>;
def GPR32Op : RegisterOperand<GPR32>;
def F0 : Register<"f0"> { let Namespace = "MyTarget"; }
def FPR32 : RegisterClass<"MyTarget", [f32], 32, (add F0)>;
diff --git a/llvm/test/TableGen/GlobalISelEmitter-implicit-defs.td b/llvm/test/TableGen/GlobalISelEmitter-implicit-defs.td
index 79af1a336f2890..36d0090b7ab163 100644
--- a/llvm/test/TableGen/GlobalISelEmitter-implicit-defs.td
+++ b/llvm/test/TableGen/GlobalISelEmitter-implicit-defs.td
@@ -3,10 +3,34 @@
include "llvm/Target/Target.td"
include "GlobalISelEmitterCommon.td"
-// CHECK: Skipped pattern: Pattern defines a physical register
let Uses = [B0], Defs = [B0] in
def tst1 : I<(outs), (ins), [(set B0, (add B0, 1))]>;
-// CHECK: Skipped pattern: Src pattern result has 1 def(s) without the HasNoUse predicate set to true but Dst MI has no def
+// CHECK: Skipped pattern: unhandled discarded def
let Uses = [B0] in
def tst2 : I<(outs), (ins), [(set B0, (add B0, 1))]>;
+
+// test courtesy @s-barannikov
+def SDTBinOpWithFlagsOut : SDTypeProfile<2, 2, [
+ SDTCisInt<0>, // result
+ SDTCisVT<1, i32>, // out flags
+ SDTCisSameAs<2, 0>, // lhs
+ SDTCisSameAs<3, 0> // rhs
+]>;
+
+def my_sub : SDNode<"MyTargetISD::SUB", SDTBinOpWithFlagsOut>;
+def my_ineg : PatFrag<(ops node:$val), (my_sub 0, node:$val)>;
+
+let Defs = [R1], Constraints = "$rd = $rs2" in
+def tst3 : I<(outs GPR32:$rd), (ins GPR32:$rs2), []>;
+
+// CHECK: Skipped pattern: Src pattern result has more defs than dst MI (2 def(s) vs 1 def(s))
+def : Pat<(my_ineg i32:$val), (tst3 i32:$val)>;
+
+def G_MY_SUB : GenericInstruction {
+ let Namespace = "MyTarget";
+ let OutOperandList = (outs type0:$dst, type1:$flags_out);
+ let InOperandList = (ins type0:$lhs, type0:$rhs);
+}
+
+def : GINodeEquiv<G_MY_SUB, my_sub>;
diff --git a/llvm/utils/TableGen/GlobalISelEmitter.cpp b/llvm/utils/TableGen/GlobalISelEmitter.cpp
index e866bd983e04ea..db8bf826cc5a7e 100644
--- a/llvm/utils/TableGen/GlobalISelEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelEmitter.cpp
@@ -398,7 +398,8 @@ class GlobalISelEmitter final : public GlobalISelMatchTableExecutorEmitter {
Expected<BuildMIAction &> createAndImportInstructionRenderer(
RuleMatcher &M, InstructionMatcher &InsnMatcher,
- const TreePatternNode &Src, const TreePatternNode &Dst);
+ const TreePatternNode &Src, const TreePatternNode &Dst,
+ ArrayRef<const Record *> DstPhysDefs);
Expected<action_iterator> createAndImportSubInstructionRenderer(
action_iterator InsertPt, RuleMatcher &M, const TreePatternNode &Dst,
const TreePatternNode &Src, unsigned TempReg);
@@ -406,11 +407,10 @@ class GlobalISelEmitter final : public GlobalISelMatchTableExecutorEmitter {
createInstructionRenderer(action_iterator InsertPt, RuleMatcher &M,
const TreePatternNode &Dst);
- Expected<action_iterator>
- importExplicitDefRenderers(action_iterator InsertPt, RuleMatcher &M,
- BuildMIAction &DstMIBuilder,
- const TreePatternNode &Src,
- const TreePatternNode &Dst, unsigned Start = 0);
+ Expected<action_iterator> importExplicitDefRenderers(
+ action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder,
+ const TreePatternNode &Src, const TreePatternNode &Dst,
+ ArrayRef<const Record *> DstPhysDefs = {}, unsigned Start = 0);
Expected<action_iterator> importExplicitUseRenderers(
action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder,
@@ -421,8 +421,6 @@ class GlobalISelEmitter final : public GlobalISelMatchTableExecutorEmitter {
Error importDefaultOperandRenderers(action_iterator InsertPt, RuleMatcher &M,
BuildMIAction &DstMIBuilder,
const DAGDefaultOperand &DefaultOp) const;
- Error importImplicitDefRenderers(BuildMIAction &DstMIBuilder,
- ArrayRef<const Record *> ImplicitDefs) const;
/// Analyze pattern \p P, returning a matcher for it if possible.
/// Otherwise, return an Error explaining why we don't support it.
@@ -1348,7 +1346,7 @@ Expected<action_iterator> GlobalISelEmitter::importExplicitUseRenderer(
Expected<BuildMIAction &> GlobalISelEmitter::createAndImportInstructionRenderer(
RuleMatcher &M, InstructionMatcher &InsnMatcher, const TreePatternNode &Src,
- const TreePatternNode &Dst) {
+ const TreePatternNode &Dst, ArrayRef<const Record *> DstPhysDefs) {
auto InsertPtOrError = createInstructionRenderer(M.actions_end(), M, Dst);
if (auto Error = InsertPtOrError.takeError())
return std::move(Error);
@@ -1367,9 +1365,9 @@ Expected<BuildMIAction &> GlobalISelEmitter::createAndImportInstructionRenderer(
CopyToPhysRegMIBuilder.addRenderer<CopyPhysRegRenderer>(PhysInput.first);
}
- if (auto Error =
- importExplicitDefRenderers(InsertPt, M, DstMIBuilder, Src, Dst)
- .takeError())
+ if (auto Error = importExplicitDefRenderers(InsertPt, M, DstMIBuilder, Src,
+ Dst, DstPhysDefs)
+ .takeError())
return std::move(Error);
if (auto Error =
@@ -1399,8 +1397,9 @@ GlobalISelEmitter::createAndImportSubInstructionRenderer(
// Handle additional (ignored) results.
if (DstMIBuilder.getCGI()->Operands.NumDefs > 1) {
- InsertPtOrError = importExplicitDefRenderers(
- std::prev(*InsertPtOrError), M, DstMIBuilder, Src, Dst, /*Start=*/1);
+ InsertPtOrError =
+ importExplicitDefRenderers(std::prev(*InsertPtOrError), M, DstMIBuilder,
+ Src, Dst, /*DstPhysDefs=*/{}, /*Start=*/1);
if (auto Error = InsertPtOrError.takeError())
return std::move(Error);
}
@@ -1533,19 +1532,29 @@ Expected<action_iterator> GlobalISelEmitter::createInstructionRenderer(
Expected<action_iterator> GlobalISelEmitter::importExplicitDefRenderers(
action_iterator InsertPt, RuleMatcher &M, BuildMIAction &DstMIBuilder,
- const TreePatternNode &Src, const TreePatternNode &Dst, unsigned Start) {
+ const TreePatternNode &Src, const TreePatternNode &Dst,
+ ArrayRef<const Record *> DstPhysDefs, unsigned Start) {
const CodeGenInstruction *DstI = DstMIBuilder.getCGI();
const unsigned SrcNumDefs = Src.getExtTypes().size();
- const unsigned DstNumDefs = DstI->Operands.NumDefs;
+ const unsigned DstNumVirtDefs = DstI->Operands.NumDefs,
+ DstNumDefs = DstNumVirtDefs + DstPhysDefs.size();
if (DstNumDefs == 0)
return InsertPt;
for (unsigned I = Start; I < SrcNumDefs; ++I) {
- std::string OpName = getMangledRootDefName(DstI->Operands[I].Name);
- // CopyRenderer saves a StringRef, so cannot pass OpName itself -
- // let's use a string with an appropriate lifetime.
- StringRef PermanentRef = M.getOperandMatcher(OpName).getSymbolicName();
- DstMIBuilder.addRenderer<CopyRenderer>(PermanentRef);
+ if (I < DstNumVirtDefs) {
+ std::string OpName = getMangledRootDefName(DstI->Operands[I].Name);
+ // CopyRenderer saves a StringRef, so cannot pass OpName itself -
+ // let's use a string with an appropriate lifetime.
+ StringRef PermanentRef = M.getOperandMatcher(OpName).getSymbolicName();
+ DstMIBuilder.addRenderer<CopyRenderer>(PermanentRef);
+ } else if (I < DstNumDefs) {
+ const auto *PhysReg = DstPhysDefs[I - DstNumVirtDefs];
+ DstMIBuilder.addRenderer<CopyPhysRegRenderer>(PhysReg);
+ } else {
+ return failedImport("number of defs in src exceeds number of implicit "
+ "and explicit defs in dst");
+ }
}
// Some instructions have multiple defs, but are missing a type entry
@@ -1788,13 +1797,6 @@ Error GlobalISelEmitter::importDefaultOperandRenderers(
return Error::success();
}
-Error GlobalISelEmitter::importImplicitDefRenderers(
- BuildMIAction &DstMIBuilder, ArrayRef<const Record *> ImplicitDefs) const {
- if (!ImplicitDefs.empty())
- return failedImport("Pattern defines a physical register");
- return Error::success();
-}
-
std::optional<const CodeGenRegisterClass *>
GlobalISelEmitter::getRegClassFromLeaf(const TreePatternNode &Leaf) {
assert(Leaf.isLeaf() && "Expected leaf?");
@@ -2022,11 +2024,9 @@ Expected<RuleMatcher> GlobalISelEmitter::runOnPattern(const PatternToMatch &P) {
auto &DstI = Target.getInstruction(DstOp);
StringRef DstIName = DstI.TheDef->getName();
-
- // Count both implicit and explicit defs in the dst instruction.
- // This avoids errors importing patterns that have inherent implicit defs.
- unsigned DstExpDefs = DstI.Operands.NumDefs,
- DstNumDefs = DstI.ImplicitDefs.size() + DstExpDefs,
+ const auto &DstPhysDefs = P.getDstRegs();
+ unsigned DstNumVirtDefs = DstI.Operands.NumDefs,
+ DstNumDefs = DstNumVirtDefs + DstPhysDefs.size(),
SrcNumDefs = Src.getExtTypes().size();
if (DstNumDefs < SrcNumDefs) {
if (DstNumDefs != 0)
@@ -2048,13 +2048,13 @@ Expected<RuleMatcher> GlobalISelEmitter::runOnPattern(const PatternToMatch &P) {
// The root of the match also has constraints on the register bank so that it
// matches the result instruction.
unsigned OpIdx = 0;
- unsigned N = std::min(DstExpDefs, SrcNumDefs);
+ unsigned N = std::min(DstNumDefs, SrcNumDefs);
for (unsigned I = 0; I < N; ++I) {
const TypeSetByHwMode &VTy = Src.getExtType(I);
- const auto &DstIOperand = DstI.Operands[OpIdx];
PointerUnion<const Record *, const CodeGenRegisterClass *> MatchedRC =
- DstIOperand.Rec;
+ OpIdx < DstNumVirtDefs ? DstI.Operands[OpIdx].Rec
+ : DstPhysDefs[OpIdx - DstNumVirtDefs];
if (DstIName == "COPY_TO_REGCLASS") {
MatchedRC = getInitValueAsRegClass(Dst.getChild(1).getLeafValue());
@@ -2092,7 +2092,13 @@ Expected<RuleMatcher> GlobalISelEmitter::runOnPattern(const PatternToMatch &P) {
MatchedRC = *MaybeRegClass;
} else if (MatchedRC.get<const Record *>()->isSubClassOf("RegisterOperand"))
MatchedRC = MatchedRC.get<const Record *>()->getValueAsDef("RegClass");
- else if (!MatchedRC.get<const Record *>()->isSubClassOf("RegisterClass"))
+ else if (MatchedRC.get<const Record *>()->isSubClassOf("Register")) {
+ auto MaybeRegClass =
+ CGRegs.getRegClassForRegister(MatchedRC.get<const Record *>());
+ if (!MaybeRegClass)
+ return failedImport("Cannot infer register class for register");
+ MatchedRC = MaybeRegClass;
+ } else if (!MatchedRC.get<const Record *>()->isSubClassOf("RegisterClass"))
return failedImport("Dst MI def isn't a register class" + to_string(Dst));
OperandMatcher &OM = InsnMatcher.getOperand(OpIdx);
@@ -2100,8 +2106,12 @@ Expected<RuleMatcher> GlobalISelEmitter::runOnPattern(const PatternToMatch &P) {
// those used in pattern's source and destination DAGs, so mangle the
// former to prevent implicitly adding unexpected
// GIM_CheckIsSameOperand predicates by the defineOperand method.
- OM.setSymbolicName(getMangledRootDefName(DstIOperand.Name));
- M.defineOperand(OM.getSymbolicName(), OM);
+ if (OpIdx < DstNumVirtDefs) {
+ OM.setSymbolicName(getMangledRootDefName(DstI.Operands[OpIdx].Name));
+ M.defineOperand(OM.getSymbolicName(), OM);
+ } else {
+ M.definePhysRegOperand(DstPhysDefs[OpIdx - DstNumVirtDefs], OM);
+ }
if (MatchedRC.is<const Record *>())
MatchedRC = &Target.getRegisterClass(MatchedRC.get<const Record *>());
OM.addPredicate<RegisterBankOperandMatcher>(
@@ -2110,16 +2120,11 @@ Expected<RuleMatcher> GlobalISelEmitter::runOnPattern(const PatternToMatch &P) {
}
auto DstMIBuilderOrError =
- createAndImportInstructionRenderer(M, InsnMatcher, Src, Dst);
+ createAndImportInstructionRenderer(M, InsnMatcher, Src, Dst, DstPhysDefs);
if (auto Error = DstMIBuilderOrError.takeError())
return std::move(Error);
BuildMIAction &DstMIBuilder = DstMIBuilderOrError.get();
- // Render the implicit defs.
- // These are only added to the root of the result.
- if (auto Error = importImplicitDefRenderers(DstMIBuilder, P.getDstRegs()))
- return std::move(Error);
-
DstMIBuilder.chooseInsnToMutate(M);
// Constrain the registers to classes. This is normally derived from the
|
JL2210
changed the title
arsenm
reviewed
Oct 21, 2024
| let Uses = [B0] in | ||
| def tst2 : I<(outs), (ins), [(set B0, (add B0, 1))]>; | ||
|
|
||
| // test courtesy @s-barannikov |
| SDTCisSameAs<3, 0> // rhs | ||
| ]>; | ||
|
|
||
| def my_sub : SDNode<"MyTargetISD::SUB", SDTBinOpWithFlagsOut>; |
Contributor
There was a problem hiding this comment.
Test with the allowed flag discard case?
Contributor
Author
There was a problem hiding this comment.
I'm not sure what you mean by that. I haven't really used these since they don't get imported. Can you give an example?
Contributor
There was a problem hiding this comment.
I thought we had a mechanism so you could write patterns to allow unused explicit outputs to be ignored in the output pattern.
| // This avoids errors importing patterns that have inherent implicit defs. | ||
| unsigned DstExpDefs = DstI.Operands.NumDefs, | ||
| DstNumDefs = DstI.ImplicitDefs.size() + DstExpDefs, | ||
| const auto &DstPhysDefs = P.getDstRegs(); |
Comment on lines
+2096
to
+2100
| auto MaybeRegClass = | ||
| CGRegs.getRegClassForRegister(MatchedRC.get<const Record *>()); | ||
| if (!MaybeRegClass) | ||
| return failedImport("Cannot infer register class for register"); | ||
| MatchedRC = MaybeRegClass; |
Contributor
There was a problem hiding this comment.
Physical registers need their own matcher checking for exact match rather than for any register in some register class.
Is there a testcase for this change?
Contributor
There was a problem hiding this comment.
Physical register inputs are copies to that physical register, not a check for an input in that physical register
Contributor
Author
|
I'm not sure this is entirely correct. I tested it on my backend that and it seems some instructions that don't use physical registers don't have their operands rendered (is that the right word?). |
don't use auto
add testcase for missing register class
arsenm
reviewed
Oct 30, 2024
Comment on lines
24
to
25
| let Defs = [R1], Constraints = "$rd = $rs2" in | ||
| def tst3 : I<(outs GPR32:$rd), (ins GPR32:$rs2), []>; |
Contributor
There was a problem hiding this comment.
Suggested change
| let Defs = [R1], Constraints = "$rd = $rs2" in | |
| def tst3 : I<(outs GPR32:$rd), (ins GPR32:$rs2), []>; | |
| def tst3 : I<(outs GPR32:$rd), (ins GPR32:$rs2), []> { | |
| let Defs = [R1]; | |
| let Constraints = "$rd = $rs2"; | |
| } |
Comment on lines
27
to
28
| // CHECK: Skipped pattern: Src pattern result has more defs than dst MI (2 def(s) vs 1 def(s)) | ||
| def : Pat<(my_ineg i32:$val), (tst3 i32:$val)>; |
Contributor
There was a problem hiding this comment.
Why is this failing to import? We should allow selecting instructions with unused implicit defs. e.g. you should be able to select an add-no-carry input node to an add-with-implicit-physreg-carry-out def, where the implicit physical def is dead and the main result is used for the virtual register output
Contributor
There was a problem hiding this comment.
The implicit def is (or, at least, supposed to be) used for the second result of the custom node.
Contributor
Author
|
I don't know if this is relevant anymore, I had some old commits on my local tree. I lost interest in the project that needed this (again) so if someone wants to pick it up be my guest. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Most of the heavy lifting was already done in
importExplicitDefRenderers, so I just added an operand for the physical register defs.The logic introduced in #112673 was indeed subtly broken; it was only supposed to count the physical register defs present in the pattern rather than all physical register defs.
Remove
importImplicitDefRendererssince it was unimplemented anyway, and I don't see a way that it could be implemented satisfactorily. Also, the name was rather misleading; it should've been called something likeimportPhysRegDefRenderers.Test courtesy of @s-barannikov