[llvm] Allow physical registers in patterns (PR #113127)
via llvm-commits
llvm-commits at lists.llvm.org
Sun Oct 20 22:40:37 PDT 2024
https://github.com/JL2210 created https://github.com/llvm/llvm-project/pull/113127
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 `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`.
Test courtesy of @s-barannikov
>From a99b8b120fca3a28f7723d493752205d35c1de5d Mon Sep 17 00:00:00 2001
From: James R Larrowe <larrowe.semaj11 at gmail.com>
Date: Sun, 20 Oct 2024 22:19:11 -0400
Subject: [PATCH 1/2] Allow physical registers in patterns
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 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
---
llvm/utils/TableGen/GlobalISelEmitter.cpp | 95 ++++++++++++-----------
1 file changed, 50 insertions(+), 45 deletions(-)
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
>From bef517e3c17f73ceec26b58310a708511c048f46 Mon Sep 17 00:00:00 2001
From: James R Larrowe <larrowe.semaj11 at gmail.com>
Date: Mon, 21 Oct 2024 01:03:59 -0400
Subject: [PATCH 2/2] Add test for implicit defs
thanks @s-barannikov!
---
.../Common/GlobalISelEmitterCommon.td | 3 +-
.../GlobalISelEmitter-implicit-defs.td | 28 +++++++++++++++++--
2 files changed, 28 insertions(+), 3 deletions(-)
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>;
More information about the llvm-commits
mailing list