[TableGen] Add InstrInfoEmitter options for operand type/size mappings #125797

jurahul · 2025-02-05T03:20:57Z

It seems operand type and operand size mappings are only used by a few backends. So disable their emission by default and have the backends that use these opt-in.

llvmbot · 2025-02-05T19:43:10Z

@llvm/pr-subscribers-tablegen
@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-backend-m68k

Author: Rahul Joshi (jurahul)

Changes

It seems operand type and operand size mappings are only used by a few backends. So disable their emission by default and have the backends that use these opt-in.

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

6 Files Affected:

(modified) llvm/lib/Target/AVR/CMakeLists.txt (+1-1)
(modified) llvm/lib/Target/M68k/CMakeLists.txt (+3-1)
(modified) llvm/lib/Target/X86/CMakeLists.txt (+2-1)
(modified) llvm/test/TableGen/get-operand-type-no-expand.td (+2-2)
(modified) llvm/test/TableGen/get-operand-type.td (+2-1)
(modified) llvm/utils/TableGen/InstrInfoEmitter.cpp (+23-8)

diff --git a/llvm/lib/Target/AVR/CMakeLists.txt b/llvm/lib/Target/AVR/CMakeLists.txt
index 817ba739d8418ac..5b25f96d866698e 100644
--- a/llvm/lib/Target/AVR/CMakeLists.txt
+++ b/llvm/lib/Target/AVR/CMakeLists.txt
@@ -7,7 +7,7 @@ tablegen(LLVM AVRGenAsmWriter.inc -gen-asm-writer)
 tablegen(LLVM AVRGenCallingConv.inc -gen-callingconv)
 tablegen(LLVM AVRGenDAGISel.inc -gen-dag-isel)
 tablegen(LLVM AVRGenDisassemblerTables.inc -gen-disassembler)
-tablegen(LLVM AVRGenInstrInfo.inc -gen-instr-info)
+tablegen(LLVM AVRGenInstrInfo.inc -gen-instr-info --emit-operand-types)
 tablegen(LLVM AVRGenMCCodeEmitter.inc -gen-emitter)
 tablegen(LLVM AVRGenRegisterInfo.inc -gen-register-info)
 tablegen(LLVM AVRGenSubtargetInfo.inc -gen-subtarget)
diff --git a/llvm/lib/Target/M68k/CMakeLists.txt b/llvm/lib/Target/M68k/CMakeLists.txt
index 1661dccece3dd8a..c41ca03122f5be4 100644
--- a/llvm/lib/Target/M68k/CMakeLists.txt
+++ b/llvm/lib/Target/M68k/CMakeLists.txt
@@ -5,7 +5,9 @@ set(LLVM_TARGET_DEFINITIONS M68k.td)
 tablegen(LLVM M68kGenGlobalISel.inc       -gen-global-isel)
 tablegen(LLVM M68kGenRegisterInfo.inc     -gen-register-info)
 tablegen(LLVM M68kGenRegisterBank.inc     -gen-register-bank)
-tablegen(LLVM M68kGenInstrInfo.inc        -gen-instr-info)
+tablegen(LLVM M68kGenInstrInfo.inc        -gen-instr-info
+                                          -emit-operand-types
+                                          -emit-logical-operand-size)
 tablegen(LLVM M68kGenSubtargetInfo.inc    -gen-subtarget)
 tablegen(LLVM M68kGenMCCodeEmitter.inc    -gen-emitter)
 tablegen(LLVM M68kGenMCPseudoLowering.inc -gen-pseudo-lowering)
diff --git a/llvm/lib/Target/X86/CMakeLists.txt b/llvm/lib/Target/X86/CMakeLists.txt
index 9553a8619feb51d..7f76e7126a59e22 100644
--- a/llvm/lib/Target/X86/CMakeLists.txt
+++ b/llvm/lib/Target/X86/CMakeLists.txt
@@ -13,7 +13,8 @@ tablegen(LLVM X86GenExegesis.inc -gen-exegesis)
 tablegen(LLVM X86GenFastISel.inc -gen-fast-isel)
 tablegen(LLVM X86GenGlobalISel.inc -gen-global-isel)
 tablegen(LLVM X86GenInstrInfo.inc -gen-instr-info
-                                  -instr-info-expand-mi-operand-info=0)
+                                  -instr-info-expand-mi-operand-info=0
+                                  -emit-operand-types)
 tablegen(LLVM X86GenMnemonicTables.inc -gen-x86-mnemonic-tables -asmwriternum=1)
 tablegen(LLVM X86GenRegisterBank.inc -gen-register-bank)
 tablegen(LLVM X86GenRegisterInfo.inc -gen-register-info)
diff --git a/llvm/test/TableGen/get-operand-type-no-expand.td b/llvm/test/TableGen/get-operand-type-no-expand.td
index 9dfcbfaec76af1e..d96738f7cb597ee 100644
--- a/llvm/test/TableGen/get-operand-type-no-expand.td
+++ b/llvm/test/TableGen/get-operand-type-no-expand.td
@@ -30,7 +30,7 @@ def InstA : Instruction {
 }
 
 // RUN: llvm-tblgen -gen-instr-info -I %p/../../include %s \
-// RUN:   -instr-info-expand-mi-operand-info=1 \
+// RUN:   -emit-operand-types -instr-info-expand-mi-operand-info=1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-EXPAND
 // CHECK-EXPAND: #ifdef GET_INSTRINFO_OPERAND_TYPE
 // CHECK-EXPAND: OpcodeOperandTypes[] = {
@@ -39,7 +39,7 @@ def InstA : Instruction {
 // CHECK-EXPAND: #endif // GET_INSTRINFO_OPERAND_TYPE
 
 // RUN: llvm-tblgen -gen-instr-info -I %p/../../include %s \
-// RUN:   -instr-info-expand-mi-operand-info=0 \
+// RUN:   -emit-operand-types -instr-info-expand-mi-operand-info=0 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-NOEXPAND
 // CHECK-NOEXPAND: #ifdef GET_INSTRINFO_OPERAND_TYPE
 // CHECK-NOEXPAND: OpcodeOperandTypes[] = {
diff --git a/llvm/test/TableGen/get-operand-type.td b/llvm/test/TableGen/get-operand-type.td
index 6ebda5cffe8af0a..a54217f9f983470 100644
--- a/llvm/test/TableGen/get-operand-type.td
+++ b/llvm/test/TableGen/get-operand-type.td
@@ -1,4 +1,5 @@
-// RUN: llvm-tblgen -gen-instr-info -I %p/../../include %s | FileCheck %s
+// RUN: llvm-tblgen -gen-instr-info -emit-operand-types \
+// RUN:      -I %p/../../include %s | FileCheck %s
 
 // Check that getOperandType has the expected info in it
 
diff --git a/llvm/utils/TableGen/InstrInfoEmitter.cpp b/llvm/utils/TableGen/InstrInfoEmitter.cpp
index 97c00ad49241978..7021107daed0445 100644
--- a/llvm/utils/TableGen/InstrInfoEmitter.cpp
+++ b/llvm/utils/TableGen/InstrInfoEmitter.cpp
@@ -41,6 +41,17 @@
 using namespace llvm;
 
 static cl::OptionCategory InstrInfoEmitterCat("Options for -gen-instr-info");
+
+static cl::opt<bool>
+    EnableOperandTypeMappings("emit-operand-types",
+                              cl::desc("Emit operand type emappings"),
+                              cl::cat(InstrInfoEmitterCat), cl::init(false));
+
+static cl::opt<bool> EnableLogicalOperandSizeMappings(
+    "emit-logical-operand-size",
+    cl::desc("Emit logical operand size emappings"),
+    cl::cat(InstrInfoEmitterCat), cl::init(false));
+
 static cl::opt<bool> ExpandMIOperandInfo(
     "instr-info-expand-mi-operand-info",
     cl::desc("Expand operand's MIOperandInfo DAG into suboperands"),
@@ -338,8 +349,8 @@ void InstrInfoEmitter::emitOperandNameMappings(
 void InstrInfoEmitter::emitOperandTypeMappings(
     raw_ostream &OS, const CodeGenTarget &Target,
     ArrayRef<const CodeGenInstruction *> NumberedInstructions) {
-
   StringRef Namespace = Target.getInstNamespace();
+
   ArrayRef<const Record *> Operands =
       Records.getAllDerivedDefinitions("Operand");
   ArrayRef<const Record *> RegisterOperands =
@@ -461,10 +472,10 @@ void InstrInfoEmitter::emitOperandTypeMappings(
       SizeToOperandName[Size].push_back(Op->getName());
   }
   OS << "  default: return 0;\n";
-  for (const auto &KV : SizeToOperandName) {
-    for (const StringRef &OperandName : KV.second)
+  for (const auto &[Size, Names] : SizeToOperandName) {
+    for (const StringRef &OperandName : Names)
       OS << "  case OpTypes::" << OperandName << ":\n";
-    OS << "    return " << KV.first << ";\n\n";
+    OS << "    return " << Size << ";\n\n";
   }
   OS << "  }\n}\n";
   OS << "} // end namespace llvm::" << Namespace << "\n";
@@ -1124,11 +1135,15 @@ void InstrInfoEmitter::run(raw_ostream &OS) {
   Timer.startTimer("Emit operand name mappings");
   emitOperandNameMappings(OS, Target, NumberedInstructions);
 
-  Timer.startTimer("Emit operand type mappings");
-  emitOperandTypeMappings(OS, Target, NumberedInstructions);
+  if (EnableOperandTypeMappings) {
+    Timer.startTimer("Emit operand type mappings");
+    emitOperandTypeMappings(OS, Target, NumberedInstructions);
+  }
 
-  Timer.startTimer("Emit logical operand size mappings");
-  emitLogicalOperandSizeMappings(OS, TargetName, NumberedInstructions);
+  if (EnableLogicalOperandSizeMappings) {
+    Timer.startTimer("Emit logical operand size mappings");
+    emitLogicalOperandSizeMappings(OS, TargetName, NumberedInstructions);
+  }
 
   Timer.startTimer("Emit logical operand type mappings");
   emitLogicalOperandTypeMappings(OS, TargetName, NumberedInstructions);

jurahul · 2025-02-05T19:43:56Z

This will hopefully help build time by avoiding unnecessary codegen for backends that don't need it. I am planning to do something similar to Operand names as well, but there we don't need an option.

s-barannikov

What's the measured impact of this change?

llvm/utils/TableGen/InstrInfoEmitter.cpp

llvm/lib/Target/AVR/CMakeLists.txt

- It seems operand type and operand size mappings are only used by a few backends. So disable their emission by default and have the backends that use these opt-in.

jurahul · 2025-02-05T19:59:18Z

What's the measured impact of this change?

I don't have one. In a downstream backend I am working on though, I did notice that these type mappings generate quite a bit of code that is unused. For operand size mappings, its specific to X86 as it looks for X86MemOperand. So, we could potentially remove the option and just bail out if getInstNamespace() != X86, but that seemed less kosher than adding an option.

jurahul · 2025-02-05T20:02:01Z

Note that this is pure build time improvement in running tablegen and nothing more, as the other backends should not be ingesting the unused code already, so this won't affect any binary size or anything else.

jurahul · 2025-02-05T21:24:02Z

What's the measured impact of this change?

I don't have one. In a downstream backend I am working on though, I did notice that these type mappings generate quite a bit of code that is unused. For operand size mappings, its specific to X86 as it looks for X86MemOperand. So, we could potentially remove the option and just bail out if getInstNamespace() != X86, but that seemed less kosher than adding an option.

The compile time improvement will be in the noise :( Just for the AMDGPU instrinfo, "Emit operand type mappings" is 0.6% and and "Emit logical operand size mappings" is 0.2%. So not that much. So really, its removing cruft from the generated file that no one uses. I'll leave up to the reviewers if its worth it given extra options added.

I also found that GET_INSTRINFO_LOGICAL_OPERAND_TYPE_MAP is not used by any upstream backend. So we could potentially drop it completely, unless it was utility in a downstream backend.

jurahul · 2025-02-06T18:19:19Z

@topperc and @s-barannikov yay or nay?

s-barannikov · 2025-02-06T18:50:24Z

Since there is no build time improvement, I'm not sure we need these options.
I'm fine with checking for X86 when generating size mappings.

jurahul force-pushed the instrinfo_emit_options branch from a1b2c7a to 03917a5 Compare February 5, 2025 16:32

jurahul marked this pull request as ready for review February 5, 2025 19:42

llvmbot added backend:X86 tablegen backend:m68k labels Feb 5, 2025

jurahul requested review from s-barannikov and topperc February 5, 2025 19:42

s-barannikov reviewed Feb 5, 2025

View reviewed changes

llvm/utils/TableGen/InstrInfoEmitter.cpp Outdated Show resolved Hide resolved

llvm/lib/Target/AVR/CMakeLists.txt Outdated Show resolved Hide resolved

[TableGen] Add InstrInfoEmitter options for operand type/size mappings

a4d3f5e

- It seems operand type and operand size mappings are only used by a few backends. So disable their emission by default and have the backends that use these opt-in.

jurahul force-pushed the instrinfo_emit_options branch from 03917a5 to a4d3f5e Compare February 5, 2025 19:55

jurahul requested a review from s-barannikov February 5, 2025 19:59

jurahul closed this Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TableGen] Add InstrInfoEmitter options for operand type/size mappings #125797

[TableGen] Add InstrInfoEmitter options for operand type/size mappings #125797

Uh oh!

jurahul commented Feb 5, 2025

Uh oh!

llvmbot commented Feb 5, 2025 •

edited

Loading

Uh oh!

jurahul commented Feb 5, 2025

Uh oh!

s-barannikov left a comment

Uh oh!

Uh oh!

Uh oh!

jurahul commented Feb 5, 2025

Uh oh!

jurahul commented Feb 5, 2025

Uh oh!

jurahul commented Feb 5, 2025

Uh oh!

jurahul commented Feb 6, 2025

Uh oh!

s-barannikov commented Feb 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[TableGen] Add InstrInfoEmitter options for operand type/size mappings #125797

[TableGen] Add InstrInfoEmitter options for operand type/size mappings #125797

Uh oh!

Conversation

jurahul commented Feb 5, 2025

Uh oh!

llvmbot commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jurahul commented Feb 5, 2025

Uh oh!

s-barannikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jurahul commented Feb 5, 2025

Uh oh!

jurahul commented Feb 5, 2025

Uh oh!

jurahul commented Feb 5, 2025

Uh oh!

jurahul commented Feb 6, 2025

Uh oh!

s-barannikov commented Feb 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

llvmbot commented Feb 5, 2025 •

edited

Loading