[TableGen][DecoderEmitter] Add option to emit type-specialized code #146593

jurahul · 2025-07-01T19:40:10Z

This change attempts to reduce the size of the disassembler code generated by DecoderEmitter.

Current state:

Currently, the code generated by the decoder emitter consists of two key functions: decodeInstruction which is the entry point into the generated code and decodeToMCInst which is invoked when a decode op is reached in the while traversing through the decoder table. Both functions are templated on InsnType which is the raw instruction bits that are supplied to decodeInstruction.
Several backends call decodeInstruction with different types, leading to several template instantiations of this function in the final code. As an example, AMDGPU instantiates this function with type DecoderUInt128 type for decoding 96/128 bit instructions, uint64_t for decoding 64-bit instructions, and uint32_t for decoding 32-bit instructions.
Since there is just one decodeToMCInst generated, it has code that handles all instruction sizes. The decoders emitted for different instructions sizes rarely have any intersection with each other. That means, in the AMDGPU case, the instantiation with InsnType == DecoderUInt128 has decoder code for 32/64-bit instructions that is never exercised. Conversely, the instantiation with InsnType == uint64_t has decoder code for 128/96/32 bit instructions that is never exercised. This leads to unnecessary dead code in the generated disassembler binary.

With this change, the DecoderEmitter will stop generating a single templated decodeInstruction and will instead generate several overloaded versions of this function and the associated decodeToMCInst function as well. Instead of using the templated InsnType, it will use an auto-inferred type which can be either a standard C++ integrer type, APInt, or a std::bitset. As a results, decoders for 32-bit instructions will appear only in the 32-bit variant of decodeToMCinst and 64-bit decoders will appear only in 64-bit variant and that will fix the code duplication in the templated variant.

Additionally, the DecodeIndex will now be computed per-instruction bitwidth as instead of being computed globally across all bitwidths in the earlier case. So, the values will generally be smaller than before and hence will consume less bytes in their ULEB128 encoding in the decoder tables, resulting in further reduction in the size of the decode tables.

Since this non-templated decoder also needs some changes in the C++ code, added an option GenerateTemplatedDecoder to InstrInfo that is defaulted to false, but targets can set to true to fall back to using templated code. The goal is to migrate all targets to use non-templated decoder and deprecate this option in future.

Adopt this feature for the AMDGPU backend. In a release build, this results in a net 35% reduction in the .text size of libLLVMAMDGPUDisassembler.so and a 5% reduction in the .rodata size. Actual numbers measured locally for a Linux x86_64 build using clang-18.1.3 toolchain are:

.text 378780 -> 244684, i.e., a 35% reduction in size
.rodata 352648 -> 334960 i.e., a 5% reduction in size

For targets that do not use multiple instantiations of decodeInstruction, opting in into this feature may not result in code/data size improvement but potential compile time improvements by avoiding the use of templated code.

topperc · 2025-07-01T20:21:28Z

What's the motivation?

jurahul · 2025-07-01T20:27:36Z

Please see the PR description that I just added. I also have data to support this, will add soon (tabulating it ATM)

jurahul · 2025-07-01T20:29:50Z

Failure is in the unit test (TableGen/VarLenDecoder.td), I need to update it.

topperc · 2025-07-01T20:39:25Z

Why can't the disassembler emitter figure out the bitwidths from the tablegen input? It already makes separate tables for each instruction size.

topperc · 2025-07-01T20:42:33Z

Why can't the disassembler emitter figure out the bitwidths from the tablegen input? It already makes separate tables for each instruction size.

Oh is it because RISC-V uses 3 bit widths, but only 2 types for DecodeInstruction?

topperc · 2025-07-01T20:50:32Z

Can we store the type in the Instruction class in the .td files like the bitwidth instead of introducing a complex command line argument?

jurahul · 2025-07-01T20:54:02Z

Right, this is POC at this which shows that the proposed optimization works. I am open to changing the interface here as well. The command line one was simple enough to not mess with tablegen instruction class etc, but that is an option, though it feels more intrusive. The command line is moderately complex and localized to the decoder emitter.

jurahul · 2025-07-01T21:07:20Z

Repeating the type per-instruction record might be redundant (and we would need more verification as well to verify for a given size, all insts of that size have the C++ type specified and its consistent). One option is to add a new InstructionTypeAndSize class that records this information, and DecoderEmitter can use that if its present else fall back to templated code. Something like

class InstructionDecoderTypeAndSize<string CPPType, list<int> Bitwidths> {
}

class InstructionDecoderTypeAndSizes<list<InstructionDecoderTypeAndSize>> {
}

and a particular backend can define a single record of type InstructionDecoderTypeAndSizes<> which the DecoderEmitter will use. This is essentially encoding the command line option as a record.

// RISCV.td
// Opt-in to non-templated deocder code.
def : InstructionDecoderTypeAndSizes<[
                InstructionDecoderTypeAndSize<"uint64_t", [48]>,
                InstructionDecoderTypeAndSize<"uint32_t", [16,32]>]>;

or more simply

class InstructionDecoderTypeAndSizes<list<string> CPPTypes, list<list<int>> Bitwidths> {
}

def : InstructionDecoderTypeAndSizes<
           [ "uint32_t", uint64_t"],
           [ [16,32],    [64]     ]>;

topperc · 2025-07-01T22:55:53Z

Repeating the type per-instruction record might be redundant (and we would need more verification as well to verify for a given size, all insts of that size have the C++ type specified and its consistent). One option is to add a new InstructionTypeAndSize class that records this information, and DecoderEmitter can use that if its present else fall back to templated code. Something like
class InstructionDecoderTypeAndSize<string CPPType, list<int> Bitwidths> {
}

class InstructionDecoderTypeAndSizes<list<InstructionDecoderTypeAndSize>> {
}
and a particular backend can define a single record of type InstructionDecoderTypeAndSizes<> which the DecoderEmitter will use. This is essentially encoding the command line option as a record.
// RISCV.td
// Opt-in to non-templated deocder code.
def : InstructionDecoderTypeAndSizes<[
                InstructionDecoderTypeAndSize<"uint64_t", [48]>,
                InstructionDecoderTypeAndSize<"uint32_t", [16,32]>]>;
or more simply
class InstructionDecoderTypeAndSizes<list<string> CPPTypes, list<list<int>> Bitwidths> {
}

def : InstructionDecoderTypeAndSizes<
           [ "uint32_t", uint64_t"],
           [ [16,32],    [64]     ]>;

RISCV uses a common base class for each of the 3 instruction sizes. Other targets may be similar.

class RVInst<dag outs, dag ins, string opcodestr, string argstr,                 
             list<dag> pattern, InstFormat format>                               
    : RVInstCommon<outs, ins, opcodestr, argstr, pattern, format> {              
  field bits<32> Inst;                                                           
  // SoftFail is a field the disassembler can use to provide a way for           
  // instructions to not match without killing the whole decode process. It is   
  // mainly used for ARM, but Tablegen expects this field to exist or it fails   
  // to build the decode table.                                                  
  field bits<32> SoftFail = 0;                                                   
  let Size = 4;                                                                  
}                                                                                
                                                                                 
class RVInst48<dag outs, dag ins, string opcodestr, string argstr,               
               list<dag> pattern, InstFormat format>                             
    : RVInstCommon<outs, ins, opcodestr, argstr, pattern, format> {              
  field bits<48> Inst;                                                           
  field bits<48> SoftFail = 0;                                                   
  let Size = 6;                                                                  
}                                                                                
                                                                                 
class RVInst64<dag outs, dag ins, string opcodestr, string argstr,               
               list<dag> pattern, InstFormat format>                             
    : RVInstCommon<outs, ins, opcodestr, argstr, pattern, format> {              
  field bits<64> Inst;                                                           
  field bits<64> SoftFail = 0;                                                   
  let Size = 8;                                                                  
}

jurahul · 2025-07-01T23:58:44Z

Repeating the type per-instruction record might be redundant (and we would need more verification as well to verify for a given size, all insts of that size have the C++ type specified and its consistent). One option is to add a new InstructionTypeAndSize class that records this information, and DecoderEmitter can use that if its present else fall back to templated code. Something like
class InstructionDecoderTypeAndSize<string CPPType, list<int> Bitwidths> {
}

class InstructionDecoderTypeAndSizes<list<InstructionDecoderTypeAndSize>> {
}
and a particular backend can define a single record of type InstructionDecoderTypeAndSizes<> which the DecoderEmitter will use. This is essentially encoding the command line option as a record.
// RISCV.td
// Opt-in to non-templated deocder code.
def : InstructionDecoderTypeAndSizes<[
                InstructionDecoderTypeAndSize<"uint64_t", [48]>,
                InstructionDecoderTypeAndSize<"uint32_t", [16,32]>]>;
or more simply
class InstructionDecoderTypeAndSizes<list<string> CPPTypes, list<list<int>> Bitwidths> {
}

def : InstructionDecoderTypeAndSizes<
           [ "uint32_t", uint64_t"],
           [ [16,32],    [64]     ]>;

RISCV uses a common base class for each of the 3 instruction sizes. Other targets may be similar.

class RVInst<dag outs, dag ins, string opcodestr, string argstr,                 
             list<dag> pattern, InstFormat format>                               
    : RVInstCommon<outs, ins, opcodestr, argstr, pattern, format> {              
  field bits<32> Inst;                                                           
  // SoftFail is a field the disassembler can use to provide a way for           
  // instructions to not match without killing the whole decode process. It is   
  // mainly used for ARM, but Tablegen expects this field to exist or it fails   
  // to build the decode table.                                                  
  field bits<32> SoftFail = 0;                                                   
  let Size = 4;                                                                  
}                                                                                
                                                                                 
class RVInst48<dag outs, dag ins, string opcodestr, string argstr,               
               list<dag> pattern, InstFormat format>                             
    : RVInstCommon<outs, ins, opcodestr, argstr, pattern, format> {              
  field bits<48> Inst;                                                           
  field bits<48> SoftFail = 0;                                                   
  let Size = 6;                                                                  
}                                                                                
                                                                                 
class RVInst64<dag outs, dag ins, string opcodestr, string argstr,               
               list<dag> pattern, InstFormat format>                             
    : RVInstCommon<outs, ins, opcodestr, argstr, pattern, format> {              
  field bits<64> Inst;                                                           
  field bits<64> SoftFail = 0;                                                   
  let Size = 8;                                                                  
}

Right, but nonetheless, we will have the type specified per instruction instance and we will still need to validate for example that for all instructions with a particular size, the type string is same. To me that seems unnecessary duplication of this information and then additional verification to make sure that it's consistent. Also, unlike the size in bytes, which is a core property of the instruction, its C++ type to represent its bits in memory seems not a core property. Many backends seems to choose the same type (for example uint64_t) for all their 16/32/48/64 bit insts. Adoption wise as well, sticking it in the per-inst record seems more invasive (for example, in our and several other downstream backends the core instruction records are auto-generated so the adoption curve for this increases further).

jurahul · 2025-07-02T00:55:36Z

Requesting not review per-se but opinion on the user interface for this optimization. Choices proposed are:

Command line option, as in this PR. +: non-intrusive in terms of .td files, -: need to parse it and parsing can be flaky.
Per instruction record carries cpp type (@topperc 's suggestion): +: No command line option parsing flakiness, -: (IMO) too invasive, I see difficulty or increased complexity in adoption for auto-generated inst defs used in several downstream backends, needs additional validation for consistent across all insts of a given size.
Option embedded as a new singleton records in the .td file: +:No command line option parsing flakiness, less intrusive than the option below (as the single def is standalone not attached to anything else), no consistency checks. -: ?

topperc · 2025-07-02T00:56:55Z

Many backends seems to choose the same type (for example uint64_t) for all their 16/32/48/64 bit insts.

I'm probably going to change to uint64_t for RISC-V. The 48-bit instructions are only used by one vendor and are relatively recent additions. I think the duplication cost just wasn't considered when they were added.

I agree adding to the Inst class might be too invasive. I still think it should be in .td files somehow. Needing to change a CMake file and replicating to GN and the other build systems when a new instruction width is added seems bad.

jurahul · 2025-07-02T01:00:21Z

Many backends seems to choose the same type (for example uint64_t) for all their 16/32/48/64 bit insts.

I'm probably going to change to uint64_t for RISC-V. The 48-bit instructions are only used by one vendor and are relatively recent additions. I think the duplication cost just wasn't considered when they were added.

I agree adding to the Inst class might be too invasive. I still think it should be in .td files somehow. Needing to change a CMake file and replicating to GN and the other build systems when a new instruction width is added seems bad.

Right, is the option #3 above palatable? We essentially encode it as a standalone record that the DecoderEmitter will look for.

topperc · 2025-07-02T01:05:34Z

Many backends seems to choose the same type (for example uint64_t) for all their 16/32/48/64 bit insts.

I'm probably going to change to uint64_t for RISC-V. The 48-bit instructions are only used by one vendor and are relatively recent additions. I think the duplication cost just wasn't considered when they were added.
I agree adding to the Inst class might be too invasive. I still think it should be in .td files somehow. Needing to change a CMake file and replicating to GN and the other build systems when a new instruction width is added seems bad.

Right, is the option #3 above palatable? We essentially encode it as a standalone record that the DecoderEmitter will look for.

Maybe it should be stored in the InstrInfo class like isLittleEndianEncoding or the Target class?

…6. NFC Insn is passed to decodeInstruction which is a template function based on the type of Insn. By using uint64_t we ensure only one version of decodeInstruction is created. This reduces the file size of RISCVDisassembler.cpp.o by ~25% in my local build. This should get even more size benefit than llvm#146593.

jurahul · 2025-07-02T03:24:58Z

InstrInfo seems reasonable. Let me rework the PR to add that

jurahul · 2025-07-02T17:16:34Z

@topperc Please let me know if this new scheme looks ok. If yes, I'll migrate the rest of the targets (Right now I just changed AARCH64 and RISCV) to use this, and add some unit tests for a final review.

topperc · 2025-07-02T17:21:50Z

@topperc Please let me know if this new scheme looks ok. If yes, I'll migrate the rest of the targets (Right now I just changed AARCH64 and RISCV) to use this, and add some unit tests for a final review.

Does this have any binary size effect on RISCV after #146619?

jurahul · 2025-07-02T17:31:10Z

@topperc Please let me know if this new scheme looks ok. If yes, I'll migrate the rest of the targets (Right now I just changed AARCH64 and RISCV) to use this, and add some unit tests for a final review.

Does this have any binary size effect on RISCV after #146619?

I have not tested. My speculation is, no binary size change but just minor compile time improvement by avoiding template specialization. I'll check and report back.

jurahul · 2025-07-02T21:01:57Z

Looks like templating adds a little bit to the code size. Building the RISCVDisassembler.cpp.o in a release config with/without this change results in the following:

Old:
196112 ./build/lib/Target/RISCV/Disassembler/CMakeFiles/LLVMRISCVDisassembler.dir/RISCVDisassembler.cpp.o 
New:
196096 ./build/lib/Target/RISCV/Disassembler/CMakeFiles/LLVMRISCVDisassembler.dir/RISCVDisassembler.cpp.o

So, 16 bytes less. Not significant though.

topperc · 2025-07-02T21:14:19Z

Looks like templating adds a little bit to the code size. Building the RISCVDisassembler.cpp.o in a release config with/without this change results in the following:
Old:
196112 ./build/lib/Target/RISCV/Disassembler/CMakeFiles/LLVMRISCVDisassembler.dir/RISCVDisassembler.cpp.o 
New:
196096 ./build/lib/Target/RISCV/Disassembler/CMakeFiles/LLVMRISCVDisassembler.dir/RISCVDisassembler.cpp.o
So, 16 bytes less. Not significant though.

Could just be a difference in the name mangling of the function name? Or are you checking the .text size?

jurahul · 2025-07-02T21:22:58Z

yeah, your guess was right. I dumped the sizes with size -A and I see:

New:

.text._ZN12_GLOBAL__N_114decodeToMCInstEjN4llvm14MCDisassembler12DecodeStatusEmRNS0_6MCInstEmPKS1_Rb     27023

.text._ZN12_GLOBAL__N_117decodeInstructionEPKhRN4llvm6MCInstEmmPKNS2_14MCDisassemblerERKNS2_15MCSubtargetInfoE        9238 

Old:                                                                                                                                                                                                                                                  

.text._ZN12_GLOBAL__N_114decodeToMCInstImEEN4llvm14MCDisassembler12DecodeStatusEjS3_T_RNS1_6MCInstEmPKS2_Rb                             27023
 .text._ZN12_GLOBAL__N_117decodeInstructionImEEN4llvm14MCDisassembler12DecodeStatusEPKhRNS1_6MCInstET_mPKS2_RKNS1_15MCSubtargetInfoE      9238

That is, text sizes are the same but mangled names are different and that likely leads to larger object file sizes.

jurahul · 2025-07-02T21:27:49Z

Note though that what you did for RISCV may not be applicable/desirable for all targets. For example, AMDGPU has 128 bit instructions, so I am assuming if we just use a 128-bit type for all instructions, we may pay a penalty in terms of the bit extraction costs (32 vs 64-bit may not be as bad).

jurahul · 2025-07-02T22:21:19Z

@topperc My question is still unanswered. WDYT of this new interface to op-in into this optimization?

github-actions · 2025-07-12T01:02:23Z

✅ With the latest revision this PR passed the C/C++ code formatter.

s-barannikov

Minor notes/suggestions

llvm/utils/TableGen/DecoderEmitter.cpp

llvm/lib/TableGen/Record.cpp

llvm/utils/TableGen/DecoderEmitter.cpp

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h

…148282)

s-barannikov

.

llvm/utils/TableGen/DecoderEmitter.cpp

…eToMCInst`

jurahul · 2025-07-18T17:16:17Z

I did one other minor change. We used to generate, for each bitwidth, a decodeInstruction<N> function as well as a decodeInstruction that just calls decodeInstruction<N>. Since we don't really use decodeInstruction<N> directly, I am just generating decodeInstruction directly now. So, for example, for 16 and 32-bit instructions, old code has:

decodeToMCInst16() {}
decodeInstruction16 {
    call decodeToMCInst16()
}
decodeInstruction(..., uint16_t insn, ...)

decodeToMCInst32
decodeInstruction32 {
    call decodeToMCInst32()
}

decodeInstruction(..., uint32_t insn, ...) {
  return decodeInstruction32(...)
}

Now it will just have:

decodeToMCInst16() {}
decodeInstruction(..., uint16_t insn, ...) {
    call decodeToMCInst16()
}

decodeToMCInst32
decodeInstruction(..., uint32_t insn, ...) {
    call decodeToMCInst32()
}

jurahul force-pushed the decoder_emitter_type_specialization branch from 30d0838 to 2d7d1dc Compare July 1, 2025 22:19

jurahul requested review from s-barannikov, mshockwave and topperc July 2, 2025 00:48

topperc mentioned this pull request Jul 2, 2025

[RISCV] Use uint64_t for Insn in getInstruction32 and getInstruction16. NFC #146619

Merged

jurahul force-pushed the decoder_emitter_type_specialization branch from 2d7d1dc to 04366ee Compare July 2, 2025 14:38

jurahul force-pushed the decoder_emitter_type_specialization branch from d51627f to 1824e68 Compare July 12, 2025 01:03

s-barannikov reviewed Jul 12, 2025

View reviewed changes

llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h Show resolved Hide resolved

jurahul force-pushed the decoder_emitter_type_specialization branch from 1824e68 to 17a507f Compare July 14, 2025 19:19

jurahul referenced this pull request Jul 14, 2025

[AMDGPU] MC support for v_fmaak_f64/v_fmamk_f64 gfx1250 intructions (#…

f090554

…148282)

jurahul force-pushed the decoder_emitter_type_specialization branch 2 times, most recently from 23e63c2 to 357a409 Compare July 14, 2025 23:33

s-barannikov reviewed Jul 15, 2025

View reviewed changes

llvm/utils/TableGen/DecoderEmitter.cpp Show resolved Hide resolved

llvm/utils/TableGen/DecoderEmitter.cpp Show resolved Hide resolved

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

jurahul requested a review from s-barannikov July 15, 2025 14:05

jurahul force-pushed the decoder_emitter_type_specialization branch from 9c3defd to 180acbb Compare July 15, 2025 22:59

jurahul marked this pull request as ready for review July 16, 2025 03:05

jurahul changed the title ~~[TableGen][DecoderEmitter] Add option to emit type-specialized decodeToMCInst~~ [TableGen][DecoderEmitter] Add option to emit type-specialized code Jul 16, 2025

s-barannikov approved these changes Jul 17, 2025

View reviewed changes

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

jurahul added 14 commits July 18, 2025 09:51

[TableGen][DecoderEmitter] Add option to emit type-specialized `decod…

f4bf65b

…eToMCInst`

Review feedback

a76d841

Specialize decodeInstruction

93f00f0

Full specialization

6216e07

Rename NonTemplatedInsnType to a more generic DecoderOption

06d0679

Revert unneeded changes

77cccad

Add a struct CPPType to track C++ type better

0e1a60b

Try fix clang-format

acf5d7e

Review feedback

b2aa61a

Flip default, fix build, and fix RISCV disassembler

e82be27

Delete unused header added in earlier version

14d505d

Fix test cases, review feedback

d7f4e73

Fix build failure

339856b

Update comment

326a481

jurahul force-pushed the decoder_emitter_type_specialization branch from 180acbb to 326a481 Compare July 18, 2025 17:10

[TableGen][DecoderEmitter] Add option to emit type-specialized code #146593

Are you sure you want to change the base?

[TableGen][DecoderEmitter] Add option to emit type-specialized code #146593

Conversation

jurahul commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc commented Jul 1, 2025

Uh oh!

jurahul commented Jul 1, 2025

Uh oh!

jurahul commented Jul 1, 2025

Uh oh!

topperc commented Jul 1, 2025

Uh oh!

topperc commented Jul 1, 2025

Uh oh!

topperc commented Jul 1, 2025

Uh oh!

jurahul commented Jul 1, 2025

Uh oh!

jurahul commented Jul 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

topperc commented Jul 1, 2025

Uh oh!

jurahul commented Jul 1, 2025

Uh oh!

jurahul commented Jul 2, 2025

Uh oh!

topperc commented Jul 2, 2025

Uh oh!

jurahul commented Jul 2, 2025

Uh oh!

topperc commented Jul 2, 2025

Uh oh!

jurahul commented Jul 2, 2025

Uh oh!

jurahul commented Jul 2, 2025

Uh oh!

topperc commented Jul 2, 2025

Uh oh!

jurahul commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jurahul commented Jul 2, 2025

Uh oh!

topperc commented Jul 2, 2025

Uh oh!

jurahul commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jurahul commented Jul 2, 2025

Uh oh!

jurahul commented Jul 2, 2025

Uh oh!

github-actions bot commented Jul 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

s-barannikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

s-barannikov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jurahul commented Jul 18, 2025

Uh oh!

Uh oh!

jurahul commented Jul 1, 2025 •

edited

Loading

jurahul commented Jul 1, 2025 •

edited

Loading

jurahul commented Jul 2, 2025 •

edited

Loading

jurahul commented Jul 2, 2025 •

edited

Loading

github-actions bot commented Jul 12, 2025 •

edited

Loading