Skip to content

[LLVM][MC][DecoderEmitter] Add support to specialize decoder per bitwidth #154865

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.

Sign up for GitHub

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
merged 3 commits into from
Sep 1, 2025
Merged

[LLVM][MC][DecoderEmitter] Add support to specialize decoder per bitwidth #154865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dab1467
Cull decoders
jurahul Aug 21, 2025
e17a431
Review feedback
jurahul Aug 29, 2025
b470278
Drop unused variable
jurahul Aug 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions llvm/include/llvm/MC/MCDecoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

#include "llvm/MC/MCDisassembler/MCDisassembler.h"
#include "llvm/Support/MathExtras.h"
#include <bitset>
#include <cassert>

namespace llvm::MCD {
Expand Down Expand Up @@ -48,6 +49,15 @@ fieldFromInstruction(const InsnType &Insn, unsigned StartBit,
return Insn.extractBitsAsZExtValue(NumBits, StartBit);
}

template <size_t N>
uint64_t fieldFromInstruction(const std::bitset<N> &Insn, unsigned StartBit,
unsigned NumBits) {
assert(StartBit + NumBits <= N && "Instruction field out of bounds!");
assert(NumBits <= 64 && "Cannot support >64-bit extractions!");
const std::bitset<N> Mask(maskTrailingOnes<uint64_t>(NumBits));
return ((Insn >> StartBit) & Mask).to_ullong();
}

// Helper function for inserting bits extracted from an encoded instruction into
// an integer-typed field.
template <typename IntType>
Expand All @@ -62,6 +72,13 @@ insertBits(IntType &field, IntType bits, unsigned startBit, unsigned numBits) {
field |= bits << startBit;
}

// InsnBitWidth is essentially a type trait used by the decoder emitter to query
// the supported bitwidth for a given type. But default, the value is 0, making
// it an invalid type for use as `InsnType` when instantiating the decoder.
// Individual targets are expected to provide specializations for these based
// on their usage.
template <typename T> static constexpr uint32_t InsnBitWidth = 0;

} // namespace llvm::MCD

#endif // LLVM_MC_MCDECODER_H
3 changes: 2 additions & 1 deletion llvm/lib/Target/AMDGPU/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ tablegen(LLVM AMDGPUGenAsmMatcher.inc -gen-asm-matcher)
tablegen(LLVM AMDGPUGenAsmWriter.inc -gen-asm-writer)
tablegen(LLVM AMDGPUGenCallingConv.inc -gen-callingconv)
tablegen(LLVM AMDGPUGenDAGISel.inc -gen-dag-isel)
tablegen(LLVM AMDGPUGenDisassemblerTables.inc -gen-disassembler)
tablegen(LLVM AMDGPUGenDisassemblerTables.inc -gen-disassembler
--specialize-decoders-per-bitwidth)
tablegen(LLVM AMDGPUGenInstrInfo.inc -gen-instr-info)
tablegen(LLVM AMDGPUGenMCCodeEmitter.inc -gen-emitter)
tablegen(LLVM AMDGPUGenMCPseudoLowering.inc -gen-pseudo-lowering)
Expand Down
37 changes: 22 additions & 15 deletions llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include "llvm/Support/Compiler.h"

using namespace llvm;
using namespace llvm::MCD;

#define DEBUG_TYPE "amdgpu-disassembler"

Expand Down Expand Up @@ -446,6 +447,14 @@ static DecodeStatus decodeVersionImm(MCInst &Inst, unsigned Imm,

#include "AMDGPUGenDisassemblerTables.inc"

// Define bitwidths for various types used to instantiate the decoder.
template <> static constexpr uint32_t llvm::MCD::InsnBitWidth<uint32_t> = 32;
template <> static constexpr uint32_t llvm::MCD::InsnBitWidth<uint64_t> = 64;
template <>
static constexpr uint32_t llvm::MCD::InsnBitWidth<std::bitset<96>> = 96;
template <>
static constexpr uint32_t llvm::MCD::InsnBitWidth<std::bitset<128>> = 128;

//===----------------------------------------------------------------------===//
//
//===----------------------------------------------------------------------===//
Expand Down Expand Up @@ -498,26 +507,24 @@ template <typename T> static inline T eatBytes(ArrayRef<uint8_t>& Bytes) {
return Res;
}

static inline DecoderUInt128 eat12Bytes(ArrayRef<uint8_t> &Bytes) {
static inline std::bitset<96> eat12Bytes(ArrayRef<uint8_t> &Bytes) {
using namespace llvm::support::endian;
assert(Bytes.size() >= 12);
uint64_t Lo =
support::endian::read<uint64_t, llvm::endianness::little>(Bytes.data());
std::bitset<96> Lo(read<uint64_t, endianness::little>(Bytes.data()));
Bytes = Bytes.slice(8);
uint64_t Hi =
support::endian::read<uint32_t, llvm::endianness::little>(Bytes.data());
std::bitset<96> Hi(read<uint32_t, endianness::little>(Bytes.data()));
Bytes = Bytes.slice(4);
return DecoderUInt128(Lo, Hi);
return (Hi << 64) | Lo;
}

static inline DecoderUInt128 eat16Bytes(ArrayRef<uint8_t> &Bytes) {
static inline std::bitset<128> eat16Bytes(ArrayRef<uint8_t> &Bytes) {
using namespace llvm::support::endian;
assert(Bytes.size() >= 16);
uint64_t Lo =
support::endian::read<uint64_t, llvm::endianness::little>(Bytes.data());
std::bitset<128> Lo(read<uint64_t, endianness::little>(Bytes.data()));
Bytes = Bytes.slice(8);
uint64_t Hi =
support::endian::read<uint64_t, llvm::endianness::little>(Bytes.data());
std::bitset<128> Hi(read<uint64_t, endianness::little>(Bytes.data()));
Bytes = Bytes.slice(8);
return DecoderUInt128(Lo, Hi);
return (Hi << 64) | Lo;
}

void AMDGPUDisassembler::decodeImmOperands(MCInst &MI,
Expand Down Expand Up @@ -600,14 +607,14 @@ DecodeStatus AMDGPUDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
// Try to decode DPP and SDWA first to solve conflict with VOP1 and VOP2
// encodings
if (isGFX1250() && Bytes.size() >= 16) {
DecoderUInt128 DecW = eat16Bytes(Bytes);
std::bitset<128> DecW = eat16Bytes(Bytes);
if (tryDecodeInst(DecoderTableGFX1250128, MI, DecW, Address, CS))
break;
Bytes = Bytes_.slice(0, MaxInstBytesNum);
}

if (isGFX11Plus() && Bytes.size() >= 12) {
DecoderUInt128 DecW = eat12Bytes(Bytes);
std::bitset<96> DecW = eat12Bytes(Bytes);

if (isGFX11() &&
tryDecodeInst(DecoderTableGFX1196, DecoderTableGFX11_FAKE1696, MI,
Expand Down Expand Up @@ -642,7 +649,7 @@ DecodeStatus AMDGPUDisassembler::getInstruction(MCInst &MI, uint64_t &Size,

} else if (Bytes.size() >= 16 &&
STI.hasFeature(AMDGPU::FeatureGFX950Insts)) {
DecoderUInt128 DecW = eat16Bytes(Bytes);
std::bitset<128> DecW = eat16Bytes(Bytes);
if (tryDecodeInst(DecoderTableGFX940128, MI, DecW, Address, CS))
break;

Expand Down
38 changes: 0 additions & 38 deletions llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,44 +32,6 @@ class MCOperand;
class MCSubtargetInfo;
class Twine;

// Exposes an interface expected by autogenerated code in
// FixedLenDecoderEmitter
class DecoderUInt128 {
private:
uint64_t Lo = 0;
uint64_t Hi = 0;

public:
DecoderUInt128() = default;
DecoderUInt128(uint64_t Lo, uint64_t Hi = 0) : Lo(Lo), Hi(Hi) {}
operator bool() const { return Lo || Hi; }
uint64_t extractBitsAsZExtValue(unsigned NumBits,
unsigned BitPosition) const {
assert(NumBits && NumBits <= 64);
assert(BitPosition < 128);
uint64_t Val;
if (BitPosition < 64)
Val = Lo >> BitPosition | Hi << 1 << (63 - BitPosition);
else
Val = Hi >> (BitPosition - 64);
return Val & ((uint64_t(2) << (NumBits - 1)) - 1);
}
DecoderUInt128 operator&(const DecoderUInt128 &RHS) const {
return DecoderUInt128(Lo & RHS.Lo, Hi & RHS.Hi);
}
DecoderUInt128 operator&(const uint64_t &RHS) const {
return *this & DecoderUInt128(RHS);
}
DecoderUInt128 operator~() const { return DecoderUInt128(~Lo, ~Hi); }
bool operator==(const DecoderUInt128 &RHS) {
return Lo == RHS.Lo && Hi == RHS.Hi;
}
bool operator!=(const DecoderUInt128 &RHS) {
return Lo != RHS.Lo || Hi != RHS.Hi;
}
bool operator!=(const int &RHS) { return *this != DecoderUInt128(RHS); }
};

//===----------------------------------------------------------------------===//
// AMDGPUDisassembler
//===----------------------------------------------------------------------===//
Expand Down
3 changes: 2 additions & 1 deletion llvm/lib/Target/RISCV/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ tablegen(LLVM RISCVGenAsmWriter.inc -gen-asm-writer)
tablegen(LLVM RISCVGenCompressInstEmitter.inc -gen-compress-inst-emitter)
tablegen(LLVM RISCVGenMacroFusion.inc -gen-macro-fusion-pred)
tablegen(LLVM RISCVGenDAGISel.inc -gen-dag-isel)
tablegen(LLVM RISCVGenDisassemblerTables.inc -gen-disassembler)
tablegen(LLVM RISCVGenDisassemblerTables.inc -gen-disassembler
--specialize-decoders-per-bitwidth)
tablegen(LLVM RISCVGenInstrInfo.inc -gen-instr-info)
tablegen(LLVM RISCVGenMCCodeEmitter.inc -gen-emitter)
tablegen(LLVM RISCVGenMCPseudoLowering.inc -gen-pseudo-lowering)
Expand Down
16 changes: 9 additions & 7 deletions llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,7 +558,7 @@ static DecodeStatus decodeXqccmpRlistS0(MCInst &Inst, uint32_t Imm,
return decodeZcmpRlist(Inst, Imm, Address, Decoder);
}

static DecodeStatus decodeCSSPushPopchk(MCInst &Inst, uint32_t Insn,
static DecodeStatus decodeCSSPushPopchk(MCInst &Inst, uint16_t Insn,
uint64_t Address,
const MCDisassembler *Decoder) {
uint32_t Rs1 = fieldFromInstruction(Insn, 7, 5);
Expand Down Expand Up @@ -700,6 +700,12 @@ static constexpr DecoderListEntry DecoderList32[]{
{DecoderTableZdinxRV32Only32, {}, "RV32-only Zdinx (Double in Integer)"},
};

// Define bitwidths for various types used to instantiate the decoder.
template <> static constexpr uint32_t llvm::MCD::InsnBitWidth<uint16_t> = 16;
template <> static constexpr uint32_t llvm::MCD::InsnBitWidth<uint32_t> = 32;
// Use uint64_t to represent 48 bit instructions.
template <> static constexpr uint32_t llvm::MCD::InsnBitWidth<uint64_t> = 48;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we need to add 64-bit RISC-V instructions in the future does this break? Will we need to use std::bitset<48> to make 48 unique?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this seems backwards. Shouldn't the decoder know what bit width it's for rather than trying to reinfer it from the type that ended up being used to represent that bit width?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@topperc yeah, if we have true 64-bit insts in future, then 48-bit insts need to use a different type. It will not break silently though, since InsnBitWidth specializations need to be defined at that time and if we have > 1 type with the same InsnBitWidth value, we will get compiler warnings about ambiguous templating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jrtc27 The decoder know the number of bits, but not the actual C++ type to be used for that bitwidth (it generates templated code and backends are free to use whatever compatible types they want to use). InsnBitWidth is a way for the particular target to communicate this intent to the decoder generated code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A previous iteration of this was trying to enforce the actual type as well, but that turned out to be much larger change (see #146593).

Copy link
Collaborator

Choose a reason for hiding this comment

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

So why not flip this as a template on uint32_t that gives the type?

Copy link
Contributor Author

@jurahul jurahul Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So something like?

template <int BitWidth>
struct TypeForBitWidth {
  using T = void;
};

template <>
struct TypeForBitWidth<32> {
  using T = uint32_t;
};
template <>
struct TypeForBitWidth<64> {
  using T = uint64_t;
};

int decodeToMCInst(TypeForBitWidth<32>::T Insn) {
  return 32;
}

int decodeToMCInst(TypeForBitWidth<64>::T Insn) {
  return 64;
}

int main() {
  uint32_t x = 0;
  std::cout << decodeToMCInst(x) << '\n';
  uint64_t y = 0;
  std::cout << decodeToMCInst(y) << '\n';
}

@topperc and @s-barannikov WDYT? It conveys the same information, but I agree its more directly expressing things. Note that with this though it won't directly work if/when we want to support > 1 bitwidth for a given type (which I did attempt in this PR but gave up because MSVC broke).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure overloading on type is a good idea. For this reason and calling the function with the wrong type issue raised here #154865 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I propose we keep the current scheme for now,


DecodeStatus RISCVDisassembler::getInstruction32(MCInst &MI, uint64_t &Size,
ArrayRef<uint8_t> Bytes,
uint64_t Address,
Expand All @@ -710,9 +716,7 @@ DecodeStatus RISCVDisassembler::getInstruction32(MCInst &MI, uint64_t &Size,
}
Size = 4;

// Use uint64_t to match getInstruction48. decodeInstruction is templated
// on the Insn type.
uint64_t Insn = support::endian::read32le(Bytes.data());
uint32_t Insn = support::endian::read32le(Bytes.data());

for (const DecoderListEntry &Entry : DecoderList32) {
if (!Entry.haveContainedFeatures(STI.getFeatureBits()))
Expand Down Expand Up @@ -758,9 +762,7 @@ DecodeStatus RISCVDisassembler::getInstruction16(MCInst &MI, uint64_t &Size,
}
Size = 2;

// Use uint64_t to match getInstruction48. decodeInstruction is templated
// on the Insn type.
uint64_t Insn = support::endian::read16le(Bytes.data());
uint16_t Insn = support::endian::read16le(Bytes.data());

for (const DecoderListEntry &Entry : DecoderList16) {
if (!Entry.haveContainedFeatures(STI.getFeatureBits()))
Expand Down
Loading