What's the motivation?

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

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

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

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?

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

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.

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]     ]>;

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;                                                                  }

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 instructioninstance 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).

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. -: ?

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.

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.

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 theInstrInfo class likeisLittleEndianEncoding or theTarget class?

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

@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 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?

@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.

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.

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?

yeah, your guess was right. I dumped the sizes withsize -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.

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).

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

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

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

decodeToMCInst16() {}decodeInstruction16 {    call decodeToMCInst16()}decodeInstruction(..., uint16_t insn, ...)decodeToMCInst32decodeInstruction32 {    call decodeToMCInst32()}decodeInstruction(..., uint32_t insn, ...) {  return decodeInstruction32(...)}

Now it will just have:

decodeToMCInst16() {}decodeInstruction(..., uint16_t insn, ...) {    call decodeToMCInst16()}decodeToMCInst32decodeInstruction(..., uint32_t insn, ...) {    call decodeToMCInst32()}