Skip to content

[TableGen][GISel] Learn to import patterns with optional defs #120470

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

Conversation

s-barannikov
Copy link
Contributor

@s-barannikov s-barannikov commented Dec 18, 2024

The number of skipped patterns reduces for ARM from 4278 to 4257.
This is the only in-tree target that makes use of OptionalDefOperand.

@llvmbot
Copy link
Member

llvmbot commented Dec 18, 2024

@llvm/pr-subscribers-tablegen

@llvm/pr-subscribers-llvm-globalisel

Author: Sergei Barannikov (s-barannikov)

Changes

Depends on #120445.


Full diff: https://github.com/llvm/llvm-project/pull/120470.diff

4 Files Affected:

  • (added) llvm/test/TableGen/GlobalISelEmitter-optional-def.td (+37)
  • (modified) llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp (+6-3)
  • (modified) llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h (+4-2)
  • (modified) llvm/utils/TableGen/GlobalISelEmitter.cpp (+18-6)
diff --git a/llvm/test/TableGen/GlobalISelEmitter-optional-def.td b/llvm/test/TableGen/GlobalISelEmitter-optional-def.td
new file mode 100644
index 00000000000000..cfb105cd0218bc
--- /dev/null
+++ b/llvm/test/TableGen/GlobalISelEmitter-optional-def.td
@@ -0,0 +1,37 @@
+// RUN: llvm-tblgen -gen-global-isel -I %p/../../include -I %p/Common < %s | FileCheck %s
+
+include "llvm/Target/Target.td"
+include "GlobalISelEmitterCommon.td"
+
+def cc_out : OptionalDefOperand<i32, (ops GPR8), (ops (i8 zero_reg))>;
+def s_cc_out : OptionalDefOperand<i32, (ops GPR8, FPR32), (ops (i8 B0), F0)>;
+
+// CHECK-LABEL: // (add:{ *:[i32] } i32:{ *:[i32] }:$rs1, i32:{ *:[i32] }:$rs2)  =>  (tst2:{ *:[i32] } i32:{ *:[i32] }:$rs1, i32:{ *:[i32] }:$rs2)
+// CHECK-NEXT: GIR_BuildRootMI, /*Opcode*/GIMT_Encode2(MyTarget::tst2),
+// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/0, // DstI[rd]
+// CHECK-NEXT: GIR_AddRegister, /*InsnID*/0, GIMT_Encode2(MyTarget::B0), /*AddRegisterRegFlags*/GIMT_Encode2(RegState::Define | RegState::Dead),
+// CHECK-NEXT: GIR_AddRegister, /*InsnID*/0, GIMT_Encode2(MyTarget::F0), /*AddRegisterRegFlags*/GIMT_Encode2(RegState::Define | RegState::Dead),
+// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/1, // rs1
+// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/2, // rs2
+// CHECK-NEXT: GIR_RootConstrainSelectedInstOperands,
+// CHECK-NEXT: // GIR_Coverage, 1,
+// CHECK-NEXT: GIR_EraseRootFromParent_Done,
+
+// CHECK-LABEL: // (imm:{ *:[i32] }):$imm  =>  (tst1:{ *:[i32] } (imm:{ *:[i32] }):$imm)
+// CHECK-NEXT: GIR_BuildRootMI, /*Opcode*/GIMT_Encode2(MyTarget::tst1),
+// CHECK-NEXT: GIR_RootToRootCopy, /*OpIdx*/0, // DstI[rd]
+// CHECK-NEXT: GIR_AddRegister, /*InsnID*/0, GIMT_Encode2(MyTarget::NoRegister), /*AddRegisterRegFlags*/GIMT_Encode2(RegState::Define | RegState::Dead),
+// CHECK-NEXT: GIR_CopyConstantAsSImm, /*NewInsnID*/0, /*OldInsnID*/0, // imm
+// CHECK-NEXT: GIR_RootConstrainSelectedInstOperands,
+// CHECK-NEXT: // GIR_Coverage, 0,
+// CHECK-NEXT: GIR_EraseRootFromParent_Done,
+
+def tst1 : I<(outs GPR32:$rd, cc_out:$s), (ins i32imm:$imm),
+             [(set GPR32:$rd, imm:$imm)]>;
+
+def tst2 : I<(outs GPR32:$rd, s_cc_out:$s), (ins GPR32:$rs1, GPR32:$rs2),
+             [(set GPR32:$rd, (add i32:$rs1, i32:$rs2))]>;
+
+// TODO: There should be more tests, but any attempt to write something
+//   more complex results in tablegen crashing somewhere in
+//   TreePatternNode::UpdateNodeType.
diff --git a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
index 15ec7e17130de4..619e7a4790c88b 100644
--- a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
+++ b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.cpp
@@ -1993,10 +1993,13 @@ void AddRegisterRenderer::emitRenderOpcodes(MatchTable &Table,
   // TODO: This is encoded as a 64-bit element, but only 16 or 32-bits are
   // really needed for a physical register reference. We can pack the
   // register and flags in a single field.
-  if (IsDef)
-    Table << MatchTable::NamedValue(2, "RegState::Define");
-  else
+  if (IsDef) {
+    Table << MatchTable::NamedValue(
+        2, IsDead ? "RegState::Define | RegState::Dead" : "RegState::Define");
+  } else {
+    assert(!IsDead && "A use cannot be dead");
     Table << MatchTable::IntValue(2, 0);
+  }
   Table << MatchTable::LineBreak;
 }
 
diff --git a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h
index 00fe073057c5c9..48ce71be677c08 100644
--- a/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h
+++ b/llvm/utils/TableGen/Common/GlobalISel/GlobalISelMatchTable.h
@@ -2091,13 +2091,15 @@ class AddRegisterRenderer : public OperandRenderer {
   unsigned InsnID;
   const Record *RegisterDef;
   bool IsDef;
+  bool IsDead;
   const CodeGenTarget &Target;
 
 public:
   AddRegisterRenderer(unsigned InsnID, const CodeGenTarget &Target,
-                      const Record *RegisterDef, bool IsDef = false)
+                      const Record *RegisterDef, bool IsDef = false,
+                      bool IsDead = false)
       : OperandRenderer(OR_Register), InsnID(InsnID), RegisterDef(RegisterDef),
-        IsDef(IsDef), Target(Target) {}
+        IsDef(IsDef), IsDead(IsDead), Target(Target) {}
 
   static bool classof(const OperandRenderer *R) {
     return R->getKind() == OR_Register;
diff --git a/llvm/utils/TableGen/GlobalISelEmitter.cpp b/llvm/utils/TableGen/GlobalISelEmitter.cpp
index 715c29c4636aee..d4c599bcab7d08 100644
--- a/llvm/utils/TableGen/GlobalISelEmitter.cpp
+++ b/llvm/utils/TableGen/GlobalISelEmitter.cpp
@@ -1451,14 +1451,10 @@ Expected<action_iterator> GlobalISelEmitter::importExplicitDefRenderers(
     const TreePatternNode &Src, const TreePatternNode &Dst, unsigned Start) {
   const CodeGenInstruction *DstI = DstMIBuilder.getCGI();
 
-  // Some instructions have multiple defs, but are missing a type entry
-  // (e.g. s_cc_out operands).
-  if (Dst.getExtTypes().size() < DstI->Operands.NumDefs)
-    return failedImport("unhandled discarded def");
-
   // Process explicit defs. The caller may have already handled the first def.
   for (unsigned I = Start, E = DstI->Operands.NumDefs; I != E; ++I) {
-    std::string OpName = getMangledRootDefName(DstI->Operands[I].Name);
+    const CGIOperandList::OperandInfo &OpInfo = DstI->Operands[I];
+    std::string OpName = getMangledRootDefName(OpInfo.Name);
 
     // If the def is used in the source DAG, forward it.
     if (M.hasOperand(OpName)) {
@@ -1469,6 +1465,22 @@ Expected<action_iterator> GlobalISelEmitter::importExplicitDefRenderers(
       continue;
     }
 
+    // A discarded explicit def may be an optional physical register.
+    // If this is the case, add the default register and mark it as dead.
+    if (OpInfo.Rec->isSubClassOf("OptionalDefOperand")) {
+      for (const TreePatternNode &DefaultOp :
+           make_pointee_range(CGP.getDefaultOperand(OpInfo.Rec).DefaultOps)) {
+        const Record *Reg = cast<DefInit>(DefaultOp.getLeafValue())->getDef();
+
+        if (!Reg->isSubClassOf("Register") && Reg->getName() != "zero_reg")
+          return failedImport("optional def is not a register");
+
+        DstMIBuilder.addRenderer<AddRegisterRenderer>(
+            Target, Reg, /*IsDef=*/true, /*IsDead=*/true);
+      }
+      continue;
+    }
+
     // The def is discarded, create a dead virtual register for it.
     const TypeSetByHwMode &ExtTy = Dst.getExtType(I);
     if (!ExtTy.isMachineValueType())

const Record *Reg = cast<DefInit>(DefaultOp.getLeafValue())->getDef();

if (!Reg->isSubClassOf("Register") && Reg->getName() != "zero_reg")
return failedImport("optional def is not a register");
Copy link
Contributor

Choose a reason for hiding this comment

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

Test the error case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a few negative tests.

@s-barannikov s-barannikov force-pushed the tablegen/gisel/optional-def-patterns branch from fcd6421 to 29baa13 Compare December 21, 2024 01:21
@s-barannikov s-barannikov merged commit a7cd660 into llvm:main Dec 21, 2024
8 checks passed
@s-barannikov s-barannikov deleted the tablegen/gisel/optional-def-patterns branch December 21, 2024 02:25
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 10, 2025
…efs (#120470)

The number of skipped patterns reduces for ARM from 4278 to 4257.
This is the only in-tree target that makes use of OptionalDefOperand.

Pull Request: llvm/llvm-project#120470
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