[llvm] [TableGen] Fixes for per-HwMode decoding problem. (PR #82201)
Jason Eckhardt via llvm-commits
llvm-commits at lists.llvm.org
Sun Feb 18 19:01:29 PST 2024
https://github.com/nvjle created https://github.com/llvm/llvm-project/pull/82201
Today, if any instruction uses EncodingInfos/EncodingByHwMode to override the default encoding, the opcode field of the decoder table is generated incorrectly. This causes failed disassemblies and other problems.
Specifically, the main correctness issue is that the EncodingID is inadvertently stored in the table rather than the actual opcode. This is caused by having set up the IndexOfInstruction map incorrectly during the loop to populate NumberedEncodings-- which is then propagated around when OpcMap is set up with a bad EncodingIDAndOpcode.
Instead, do away with IndexOfInstruction altogether and use opcode value queried from CodeGenTarget::getInstrIntValue to set up OpcMap. This itself exposed another problem where emitTable was using the decoded opcode to index into NumberedEncodings. Instead pass in the EncodingIDAndOpcode vector, and create the reverse mapping from Opcode to EncodingID, which is then used to index NumberedEncodings.
This problem is not currently exposed upstream since no in-tree targets yet use the per-HwMode feature. It does show up in at least two downstream targets.
>From c56c053302f9d59ba15942af38472286050440d5 Mon Sep 17 00:00:00 2001
From: Jason Eckhardt <jeckhardt at nvidia.com>
Date: Sun, 18 Feb 2024 20:10:16 -0600
Subject: [PATCH] [TableGen] Fixes for per-HwMode decoding problem.
Today, if any instruction uses EncodingInfos/EncodingByHwMode to override
the default encoding, the opcode field of the decoder table is generated
incorrectly. This causes failed disassemblies and other problems.
Specifically, the main correctness issue is that the EncodingID is
inadvertently stored in the table rather than the actual opcode. This is
caused by having set up the IndexOfInstruction map incorrectly during
the loop to populate NumberedEncodings-- which is then propagated around
when OpcMap is set up with a bad EncodingIDAndOpcode.
Instead, do away with IndexOfInstruction altogether and use opcode value
queried from CodeGenTarget::getInstrIntValue to set up OpcMap. This itself
exposed another problem where emitTable was using the decoded opcode to
index into NumberedEncodings. Instead pass in the EncodingIDAndOpcode
vector, and create the reverse mapping from Opcode to EncodingID, which
is then used to index NumberedEncodings.
This problem is not currently exposed upstream since no in-tree targets
yet use the per-HwMode feature. It does show up in at least two downstream
targets.
---
llvm/test/TableGen/HwModeEncodeDecode.td | 5 ++--
llvm/utils/TableGen/DecoderEmitter.cpp | 35 ++++++++++++++++--------
2 files changed, 25 insertions(+), 15 deletions(-)
diff --git a/llvm/test/TableGen/HwModeEncodeDecode.td b/llvm/test/TableGen/HwModeEncodeDecode.td
index f8fcfafdd968fa..aaacd7d3f6590a 100644
--- a/llvm/test/TableGen/HwModeEncodeDecode.td
+++ b/llvm/test/TableGen/HwModeEncodeDecode.td
@@ -74,9 +74,8 @@ def baz : Instruction {
// DECODER-DAG: Opcode: fooTypeEncA:foo
// DECODER-DAG: Opcode: bar
// DECODER-LABEL: DecoderTable_ModeB32[] =
-// Note that the comment says fooTypeEncA but this is actually fooTypeEncB; plumbing
-// the correct comment text through the decoder is nontrivial.
-// DECODER-DAG: Opcode: fooTypeEncA:foo
+// DECODER-DAG: Opcode: fooTypeEncB:foo
+// DECODER-DAG: Opcode: fooTypeEncA:baz
// DECODER-DAG: Opcode: bar
// ENCODER-LABEL: static const uint64_t InstBits_ModeA[] = {
diff --git a/llvm/utils/TableGen/DecoderEmitter.cpp b/llvm/utils/TableGen/DecoderEmitter.cpp
index 02d9527fd9ed91..22a71065134a4e 100644
--- a/llvm/utils/TableGen/DecoderEmitter.cpp
+++ b/llvm/utils/TableGen/DecoderEmitter.cpp
@@ -118,6 +118,8 @@ struct EncodingIDAndOpcode {
: EncodingID(EncodingID), Opcode(Opcode) {}
};
+using EncodingIDsVec = std::vector<EncodingIDAndOpcode>;
+
raw_ostream &operator<<(raw_ostream &OS, const EncodingAndInst &Value) {
if (Value.EncodingDef != Value.Inst->TheDef)
OS << Value.EncodingDef->getName() << ":";
@@ -135,8 +137,8 @@ class DecoderEmitter {
// Emit the decoder state machine table.
void emitTable(formatted_raw_ostream &o, DecoderTable &Table,
- unsigned Indentation, unsigned BitWidth,
- StringRef Namespace) const;
+ unsigned Indentation, unsigned BitWidth, StringRef Namespace,
+ const EncodingIDsVec &EncodingIDs) const;
void emitInstrLenTable(formatted_raw_ostream &OS,
std::vector<unsigned> &InstrLen) const;
void emitPredicateFunction(formatted_raw_ostream &OS,
@@ -766,7 +768,16 @@ unsigned Filter::usefulness() const {
// Emit the decoder state machine table.
void DecoderEmitter::emitTable(formatted_raw_ostream &OS, DecoderTable &Table,
unsigned Indentation, unsigned BitWidth,
- StringRef Namespace) const {
+ StringRef Namespace,
+ const EncodingIDsVec &EncodingIDs) const {
+ // We'll need to be able to map from a decoded opcode into the corresponding
+ // EncodingID for this specific combination of BitWidth and Namespace. This
+ // is used below to index into NumberedEncodings.
+ DenseMap<unsigned, unsigned> OpcodeToEncodingID;
+ OpcodeToEncodingID.reserve(EncodingIDs.size());
+ for (auto &EI : EncodingIDs)
+ OpcodeToEncodingID[EI.Opcode] = EI.EncodingID;
+
OS.indent(Indentation) << "static const uint8_t DecoderTable" << Namespace
<< BitWidth << "[] = {\n";
@@ -888,8 +899,12 @@ void DecoderEmitter::emitTable(formatted_raw_ostream &OS, DecoderTable &Table,
// Decoder index.
I += emitULEB128(I, OS);
+ auto EncI = OpcodeToEncodingID.find(Opc);
+ assert(EncI != OpcodeToEncodingID.end() && "no encoding entry");
+ auto EncodingID = EncI->second;
+
if (!IsTry) {
- OS << "// Opcode: " << NumberedEncodings[Opc] << "\n";
+ OS << "// Opcode: " << NumberedEncodings[EncodingID] << "\n";
break;
}
@@ -899,7 +914,7 @@ void DecoderEmitter::emitTable(formatted_raw_ostream &OS, DecoderTable &Table,
uint32_t NumToSkip = emitNumToSkip(I, OS);
I += 3;
- OS << "// Opcode: " << NumberedEncodings[Opc]
+ OS << "// Opcode: " << NumberedEncodings[EncodingID]
<< ", skip to: " << ((I - Table.begin()) + NumToSkip) << "\n";
break;
}
@@ -2449,11 +2464,8 @@ void DecoderEmitter::run(raw_ostream &o) {
std::set<StringRef> HwModeNames;
const auto &NumberedInstructions = Target.getInstructionsByEnumValue();
NumberedEncodings.reserve(NumberedInstructions.size());
- DenseMap<Record *, unsigned> IndexOfInstruction;
// First, collect all HwModes referenced by the target.
for (const auto &NumberedInstruction : NumberedInstructions) {
- IndexOfInstruction[NumberedInstruction->TheDef] = NumberedEncodings.size();
-
if (const RecordVal *RV =
NumberedInstruction->TheDef->getValue("EncodingInfos")) {
if (auto *DI = dyn_cast_or_null<DefInit>(RV->getValue())) {
@@ -2470,8 +2482,6 @@ void DecoderEmitter::run(raw_ostream &o) {
HwModeNames.insert("");
for (const auto &NumberedInstruction : NumberedInstructions) {
- IndexOfInstruction[NumberedInstruction->TheDef] = NumberedEncodings.size();
-
if (const RecordVal *RV =
NumberedInstruction->TheDef->getValue("EncodingInfos")) {
if (DefInit *DI = dyn_cast_or_null<DefInit>(RV->getValue())) {
@@ -2544,7 +2554,7 @@ void DecoderEmitter::run(raw_ostream &o) {
DecoderNamespace +=
std::string("_") + NumberedEncodings[i].HwModeName.str();
OpcMap[std::pair(DecoderNamespace, Size)].emplace_back(
- i, IndexOfInstruction.find(Def)->second);
+ i, Target.getInstrIntValue(Def));
} else {
NumEncodingsOmitted++;
}
@@ -2577,7 +2587,8 @@ void DecoderEmitter::run(raw_ostream &o) {
TableInfo.Table.push_back(MCD::OPC_Fail);
// Print the table to the output stream.
- emitTable(OS, TableInfo.Table, 0, FC.getBitWidth(), Opc.first.first);
+ emitTable(OS, TableInfo.Table, 0, FC.getBitWidth(), Opc.first.first,
+ Opc.second);
}
// For variable instruction, we emit a instruction length table
More information about the llvm-commits
mailing list