[RISCV][NFC] Merge Xqci Decoder Tables #128140
Conversation
|
@llvm/pr-subscribers-backend-risc-v Author: Sam Elliott (lenary) ChangesRISC-V has multiple decoder tables because there is no guarantee that non-standard extensions do not overlap with each other. Qualcomm's Xqci family of extensions are intended to be implemented together, and therefore we want a single decode table for this group of extensions. This should be more efficient overall, and allows us to use tablegen's existing mechanism that finds overlapping encodings within the group. To implement this, the key addition is Full diff: https://github.com/llvm/llvm-project/pull/128140.diff 2 Files Affected:
diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index 01648ec0cbe9e..75f834acbc1e8 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -606,6 +606,8 @@ void RISCVDisassembler::addSPOperands(MCInst &MI) const {
(void)nullptr)
#define TRY_TO_DECODE_FEATURE(FEATURE, DECODER_TABLE, DESC) \
TRY_TO_DECODE(STI.hasFeature(FEATURE), DECODER_TABLE, DESC)
+#define TRY_TO_DECODE_FEATURE_ANY(FEATURES, DECODER_TABLE, DESC) \
+ TRY_TO_DECODE((STI.getFeatureBits() & (FEATURES)).any(), DECODER_TABLE, DESC)
DecodeStatus RISCVDisassembler::getInstruction32(MCInst &MI, uint64_t &Size,
ArrayRef<uint8_t> Bytes,
@@ -701,26 +703,15 @@ DecodeStatus RISCVDisassembler::getInstruction32(MCInst &MI, uint64_t &Size,
"CORE-V SIMD extensions custom opcode table");
TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXCVbi, DecoderTableXCVbi32,
"CORE-V Immediate Branching custom opcode table");
- TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXqcicsr, DecoderTableXqcicsr32,
- "Qualcomm uC CSR custom opcode table");
- TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXqcisls, DecoderTableXqcisls32,
- "Qualcomm uC Scaled Load Store custom opcode table");
- TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXqcia, DecoderTableXqcia32,
- "Qualcomm uC Arithmetic custom opcode table");
- TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXqcics, DecoderTableXqcics32,
- "Qualcomm uC Conditional Select custom opcode table");
- TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXqcilsm, DecoderTableXqcilsm32,
- "Qualcomm uC Load Store Multiple custom opcode table");
- TRY_TO_DECODE_FEATURE(
- RISCV::FeatureVendorXqciac, DecoderTableXqciac32,
- "Qualcomm uC Load-Store Address Calculation custom opcode table");
- TRY_TO_DECODE_FEATURE(
- RISCV::FeatureVendorXqcicli, DecoderTableXqcicli32,
- "Qualcomm uC Conditional Load Immediate custom opcode table");
- TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXqcicm, DecoderTableXqcicm32,
- "Qualcomm uC Conditional Move custom opcode table");
- TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXqciint, DecoderTableXqciint32,
- "Qualcomm uC Interrupts custom opcode table");
+
+ FeatureBitset XqciFeaturesWith32BitInsts = {
+ RISCV::FeatureVendorXqcicsr, RISCV::FeatureVendorXqcisls,
+ RISCV::FeatureVendorXqcia, RISCV::FeatureVendorXqcics,
+ RISCV::FeatureVendorXqcilsm, RISCV::FeatureVendorXqciac,
+ RISCV::FeatureVendorXqcicli, RISCV::FeatureVendorXqcicm,
+ RISCV::FeatureVendorXqciint};
+ TRY_TO_DECODE_FEATURE_ANY(XqciFeaturesWith32BitInsts, DecoderTableXqci32,
+ "Qualcomm uC Extensions");
TRY_TO_DECODE(true, DecoderTable32, "RISCV32 table");
return MCDisassembler::Fail;
@@ -747,14 +738,13 @@ DecodeStatus RISCVDisassembler::getInstruction16(MCInst &MI, uint64_t &Size,
TRY_TO_DECODE_FEATURE(
RISCV::FeatureStdExtZcmp, DecoderTableRVZcmp16,
"Zcmp table (16-bit Push/Pop & Double Move Instructions)");
- TRY_TO_DECODE_FEATURE(
- RISCV::FeatureVendorXqciac, DecoderTableXqciac16,
- "Qualcomm uC Load-Store Address Calculation custom 16bit opcode table");
- TRY_TO_DECODE_FEATURE(
- RISCV::FeatureVendorXqcicm, DecoderTableXqcicm16,
- "Qualcomm uC Conditional Move custom 16bit opcode table");
- TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXqciint, DecoderTableXqciint16,
- "Qualcomm uC Interrupts custom 16bit opcode table");
+
+ FeatureBitset XqciFeaturesWith16BitInsts = {RISCV::FeatureVendorXqciac,
+ RISCV::FeatureVendorXqcicm,
+ RISCV::FeatureVendorXqciint};
+ TRY_TO_DECODE_FEATURE_ANY(XqciFeaturesWith16BitInsts, DecoderTableXqci16,
+ "Qualcomm uC 16bit");
+
TRY_TO_DECODE_AND_ADD_SP(STI.hasFeature(RISCV::FeatureVendorXwchc),
DecoderTableXwchc16,
"WCH QingKe XW custom opcode table");
@@ -779,7 +769,7 @@ DecodeStatus RISCVDisassembler::getInstruction48(MCInst &MI, uint64_t &Size,
Insn += (static_cast<uint64_t>(Bytes[i]) << 8 * i);
}
TRY_TO_DECODE_FEATURE(
- RISCV::FeatureVendorXqcilo, DecoderTableXqcilo48,
+ RISCV::FeatureVendorXqcilo, DecoderTableXqci48,
"Qualcomm uC Large Offset Load Store custom 48bit opcode table");
return MCDisassembler::Fail;
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
index 1f042b0f47e96..5636849116e76 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXqci.td
@@ -249,7 +249,7 @@ class QCIRVInstESStore<bits<3> funct3, bits<2> funct2, string opcodestr>
// Instructions
//===----------------------------------------------------------------------===//
-let Predicates = [HasVendorXqcicsr, IsRV32], DecoderNamespace = "Xqcicsr" in {
+let Predicates = [HasVendorXqcicsr, IsRV32], DecoderNamespace = "Xqci" in {
let hasSideEffects = 1, mayLoad = 0, mayStore = 0 in {
def QC_CSRRWR : RVInstR<0b1000110, 0b000, OPC_SYSTEM, (outs GPR:$rd),
(ins GPR:$rs1, GPRNoX0:$rs2), "qc.csrrwr",
@@ -259,9 +259,9 @@ let hasSideEffects = 1, mayLoad = 0, mayStore = 0 in {
(ins uimm5:$rs1, GPRNoX0:$rs2), "qc.csrrwri",
"$rd, $rs1, $rs2">;
} // hasSideEffects = 1, mayLoad = 0, mayStore = 0
-} // Predicates = [HasVendorXqcicsr, IsRV32], DecoderNamespace = "Xqcicsr"
+} // Predicates = [HasVendorXqcicsr, IsRV32], DecoderNamespace = "Xqci"
-let Predicates = [HasVendorXqcisls, IsRV32], DecoderNamespace = "Xqcisls" in {
+let Predicates = [HasVendorXqcisls, IsRV32], DecoderNamespace = "Xqci" in {
def QC_LRB : QCILoad_ScaleIdx<0b1000, "qc.lrb">;
def QC_LRH : QCILoad_ScaleIdx<0b1001, "qc.lrh">;
def QC_LRW : QCILoad_ScaleIdx<0b1010, "qc.lrw">;
@@ -271,9 +271,9 @@ let Predicates = [HasVendorXqcisls, IsRV32], DecoderNamespace = "Xqcisls" in {
def QC_SRB : QCIStore_ScaleIdx<0b1101, "qc.srb">;
def QC_SRH : QCIStore_ScaleIdx<0b1110, "qc.srh">;
def QC_SRW : QCIStore_ScaleIdx<0b1111, "qc.srw">;
-} // Predicates = [HasVendorXqcisls, IsRV32], DecoderNamespace = "Xqcisls"
+} // Predicates = [HasVendorXqcisls, IsRV32], DecoderNamespace = "Xqci"
-let Predicates = [HasVendorXqcia, IsRV32], DecoderNamespace = "Xqcia" in {
+let Predicates = [HasVendorXqcia, IsRV32], DecoderNamespace = "Xqci" in {
let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
def QC_SLASAT : QCIRVInstRR<0b01010, GPRNoX0, "qc.slasat">;
def QC_SLLSAT : QCIRVInstRR<0b01100, GPRNoX0, "qc.sllsat">;
@@ -295,9 +295,9 @@ let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
def QC_NORMU : QCIRVInstR<0b1000, "qc.normu">;
def QC_NORMEU : QCIRVInstR<0b1001, "qc.normeu">;
} // hasSideEffects = 0, mayLoad = 0, mayStore = 0
-} // Predicates = [HasVendorXqcia, IsRV32], DecoderNamespace = "Xqcia"
+} // Predicates = [HasVendorXqcia, IsRV32], DecoderNamespace = "Xqci"
-let Predicates = [HasVendorXqciac, IsRV32], DecoderNamespace = "Xqciac" in {
+let Predicates = [HasVendorXqciac, IsRV32], DecoderNamespace = "Xqci" in {
let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
def QC_C_MULIADD : RVInst16CL<0b001, 0b10, (outs GPRC:$rd_wb),
(ins GPRC:$rd, GPRC:$rs1, uimm5:$uimm),
@@ -326,9 +326,9 @@ let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
}
} // hasSideEffects = 0, mayLoad = 0, mayStore = 0
-} // Predicates = [HasVendorXqciac, IsRV32], DecoderNamespace = "Xqciac"
+} // Predicates = [HasVendorXqciac, IsRV32], DecoderNamespace = "Xqci"
-let Predicates = [HasVendorXqcics, IsRV32], DecoderNamespace = "Xqcics" in {
+let Predicates = [HasVendorXqcics, IsRV32], DecoderNamespace = "Xqci" in {
def QC_SELECTIIEQ : QCISELECTIICC <0b010, "qc.selectiieq">;
def QC_SELECTIINE : QCISELECTIICC <0b011, "qc.selectiine">;
def QC_SELECTIEQ : QCISELECTICC <0b010, "qc.selectieq">;
@@ -337,9 +337,9 @@ let Predicates = [HasVendorXqcics, IsRV32], DecoderNamespace = "Xqcics" in {
def QC_SELECTNEI : QCISELECTCCI <0b011, "qc.selectnei">;
def QC_SELECTIEQI : QCISELECTICCI <0b010, "qc.selectieqi">;
def QC_SELECTINEI : QCISELECTICCI <0b011, "qc.selectinei">;
-} // Predicates = [HasVendorXqcics, IsRV32], DecoderNamespace = "Xqcics"
+} // Predicates = [HasVendorXqcics, IsRV32], DecoderNamespace = "Xqci"
-let Predicates = [HasVendorXqcilsm, IsRV32], DecoderNamespace = "Xqcilsm" in {
+let Predicates = [HasVendorXqcilsm, IsRV32], DecoderNamespace = "Xqci" in {
def QC_SWM : QCIStoreMultiple<0b00, GPRNoX0, "qc.swm">;
def QC_SWMI : QCIStoreMultiple<0b01, uimm5nonzero, "qc.swmi">;
def QC_SETWM : QCIStoreMultiple<0b10, GPRNoX0, "qc.setwm">;
@@ -347,9 +347,9 @@ let Predicates = [HasVendorXqcilsm, IsRV32], DecoderNamespace = "Xqcilsm" in {
def QC_LWM : QCILoadMultiple<0b00, GPRNoX0, "qc.lwm">;
def QC_LWMI : QCILoadMultiple<0b01, uimm5nonzero, "qc.lwmi">;
-} // Predicates = [HasVendorXqcilsm, IsRV32], DecoderNamespace = "Xqcilsm"
+} // Predicates = [HasVendorXqcilsm, IsRV32], DecoderNamespace = "Xqci"
-let Predicates = [HasVendorXqcicli, IsRV32], DecoderNamespace = "Xqcicli" in {
+let Predicates = [HasVendorXqcicli, IsRV32], DecoderNamespace = "Xqci" in {
def QC_LIEQ : QCILICC<0b000, 0b01, GPRNoX0, "qc.lieq">;
def QC_LINE : QCILICC<0b001, 0b01, GPRNoX0, "qc.line">;
def QC_LILT : QCILICC<0b100, 0b01, GPRNoX0, "qc.lilt">;
@@ -363,9 +363,9 @@ let Predicates = [HasVendorXqcicli, IsRV32], DecoderNamespace = "Xqcicli" in {
def QC_LIGEI : QCILICC<0b101, 0b11, simm5, "qc.ligei">;
def QC_LILTUI : QCILICC<0b110, 0b11, uimm5, "qc.liltui">;
def QC_LIGEUI : QCILICC<0b111, 0b11, uimm5, "qc.ligeui">;
-} // Predicates = [HasVendorXqcicli, IsRV32], DecoderNamespace = "Xqcicli"
+} // Predicates = [HasVendorXqcicli, IsRV32], DecoderNamespace = "Xqci"
-let Predicates = [HasVendorXqcicm, IsRV32], DecoderNamespace = "Xqcicm" in {
+let Predicates = [HasVendorXqcicm, IsRV32], DecoderNamespace = "Xqci" in {
let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
def QC_C_MVEQZ : RVInst16CL<0b101, 0b10, (outs GPRC:$rd_wb),
(ins GPRC:$rd, GPRC:$rs1),
@@ -389,9 +389,9 @@ let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
def QC_MVGEI : QCIMVCCI<0b101, "qc.mvgei", simm5>;
def QC_MVLTUI : QCIMVCCI<0b110, "qc.mvltui", uimm5>;
def QC_MVGEUI : QCIMVCCI<0b111, "qc.mvgeui", uimm5>;
-} // Predicates = [HasVendorXqcicm, IsRV32], DecoderNamespace = "Xqcicm"
+} // Predicates = [HasVendorXqcicm, IsRV32], DecoderNamespace = "Xqci"
-let Predicates = [HasVendorXqciint, IsRV32], DecoderNamespace = "Xqciint" in {
+let Predicates = [HasVendorXqciint, IsRV32], DecoderNamespace = "Xqci" in {
let hasSideEffects = 1, mayLoad = 0, mayStore = 0 in
def QC_C_DIR : RVInst16CI<0b000, 0b10, (outs GPRNoX0:$rd), (ins),
"qc.c.dir", "$rd"> {
@@ -421,9 +421,9 @@ let Predicates = [HasVendorXqciint, IsRV32], DecoderNamespace = "Xqciint" in {
let mayLoad = 1, mayStore = 1, isReturn = 1, isTerminator = 1 in
def QC_C_MILEAVERET : QCIRVInst16CI_NONE<0b10100, "qc.c.mileaveret">;
-} // Predicates = [HasVendorXqciint, IsRV32], DecoderNamespace = "Xqciint"
+} // Predicates = [HasVendorXqciint, IsRV32], DecoderNamespace = "Xqci"
-let Predicates = [HasVendorXqcilo, IsRV32], DecoderNamespace = "Xqcilo" in {
+let Predicates = [HasVendorXqcilo, IsRV32], DecoderNamespace = "Xqci" in {
def QC_E_LB : QCIRVInstEILoad<0b101, 0b00, "qc.e.lb">;
def QC_E_LBU : QCIRVInstEILoad<0b101, 0b01, "qc.e.lbu">;
def QC_E_LH : QCIRVInstEILoad<0b101, 0b10, "qc.e.lh">;
@@ -433,7 +433,7 @@ let Predicates = [HasVendorXqcilo, IsRV32], DecoderNamespace = "Xqcilo" in {
def QC_E_SB : QCIRVInstESStore<0b110, 0b01, "qc.e.sb">;
def QC_E_SH : QCIRVInstESStore<0b110, 0b10, "qc.e.sh">;
def QC_E_SW : QCIRVInstESStore<0b110, 0b11, "qc.e.sw">;
-} // Predicates = [HasVendorXqcilo, IsRV32], DecoderNamespace = "Xqcilo"
+} // Predicates = [HasVendorXqcilo, IsRV32], DecoderNamespace = "Xqci"
//===----------------------------------------------------------------------===//
// Aliases
|
|
It's less clear to me what the other vendors want to do here, but I've done it this way so they can use the same macro eventually. |
|
This might run into issues to do with what will look, to llvm, like overlapping encodings, where the encoding space in e.g. an instruction that takes GPRNoX0 is reused. We have a few places like this, but I'm not sure they're all upstream, so I will need to check that over. |
|
LGTM |
If I'm reading this right, that would be a non-conforming extension. The conflict wouldn't be with other qc extensions, but with the standard ones right? If so, having a single qc table should still be fine, you just need to check it before the standard table. Though, having the qc table split into conforming and non-conforming also doesn't seem unreasonable, which is what I think you meant. So either option works. :) |
Sorry, I was afk on Friday. Xqci and its sub-extensions are conforming, as far as I can tell. The problem is somewhat harder to describe, but @svs-quic found me an example which illustrates things better than I've been able to before. We have This is even more of an issue, in another pair of instructions from the Xqci spec - You get similar issues for e.g. GPRNoX0 fields where the encoding of all zeroes for that 5-bit field is used for something else, but I don't have a motivating example to hand. LLVM already contains the right logic for dealing with this - we just need to start using it - it's the I think if we solve all of this for Xqci (which does seem to have a lot of these "N-bit fields but not these specific encoding values" fields), this should make our decoders more accurate, and also will make it easier for other people to merge tables. One of the difficulties in the short term is not all of Xqci is upstream, so it's hard to know where/when this will bite us. That's the big reason I deprioritised this patch wrt others we're working on. |
I think for the GPRNoX0 case, if the field is constant a 0 in another instruction decoder should handle that correctly without |
Yeah, I looked closer at the decoder emitter in tablegen, and it copes fine with this and with the Zcmt I think these cases should apply to I am still not entirely enamoured by using |
RISC-V has multiple decoder tables because there is no guarantee that non-standard extensions do not overlap with each other. Qualcomm's Xqci family of extensions are intended to be implemented together, and therefore we want a single decode table for this group of extensions. This should be more efficient overall. To implement this, the key addition is `TRY_TO_DECODE_FEATURE_ANY`, which will use the provided decoder table if any of the features from the FeatureBitset (first argument) are enabled, rather than if all are enabled.
lenary
force-pushed
the
pr/merge-xqci-decoder-tables
branch
from
2d3262e to
0a2ce34
Compare
|
Given the above, I have just rebased my changes over Craig's and Harsh's. |
lenary
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXqciint, DecoderTableXqciint32, | ||
| "Qualcomm uC Interrupts"); | ||
|
|
||
| FeatureBitset XqciFeaturesWith32BitInsts = { |
There was a problem hiding this comment.
Yeah I think I might promote this up to a global static constexpr containing all the Xqci extensions, rather than splitting them by size.
6 participants
RISC-V has multiple decoder tables because there is no guarantee that non-standard extensions do not overlap with each other.
Qualcomm's Xqci family of extensions are intended to be implemented together, and therefore we want a single decode table for this group of extensions. This should be more efficient overall, and allows us to use tablegen's existing mechanism that finds overlapping encodings within the group.
To implement this, the key addition is
TRY_TO_DECODE_FEATURE_ANY, which will use the provided decoder table if any of the features from the FeatureBitset (first argument) are enabled, rather than if all are enabled.