[RISCV] Consolidate some DecoderNamespaces for standard extensions. #128954

topperc · 2025-02-26T22:49:12Z

First thing to know is that the subtarget feature checks used to block accessing a decoder table are only a performance optimization and not required for functionality. The tables have their own predicate checks. I've removed them from all the standard extension tables.

-RV32 Zacas decoder namespace has been renamed to RV32GPRPair, I think Zilsd(rv32 load/store pair) can go in here too.
-The RV32 Zdinx table has been renamed to also use RV32GPRPair.
-The Zfinx table has been renamed to remove superflous "RV" prefix.
-Zcmp and Zcmt tables have been combined into a ZcOverlap table. I think
Zclsd(rv32 compressed load/store pair) can go in here too.
-All the extra standard extension tables are checked after the main
standard extension table. This makes the common case of the main table
matching occur earlier.
-Zicfiss is the exception to this as it needs to be checked before
the main table since it overrides some encodings from Zcmop. This
can't be handled by a predicate based priority as Zicfiss only overrides
a subset of Zcmop encodings.

First thing to know is that the subtarget feature checks used to block accessing a decoder table are only a performance optimization and not required for functionality. The tables have their own predicate checks. I've removed them from all the standard extension tables. -RV32 Zacas decoder namespace has been renamed to RV32GPRPair, I think Zilsd(rv32 load/store pair) can go in here too. -The RV32 Zdinx table has been renamed to also use RV32GPRPair. -The Zfinx table has been renamed to remove superflous "RV" prefix. -Zcmp and Zcmp tables have been combined into a ZcOverlap table, I think Zclsd(rv32 compressed load/store pair) can go in here too. -All the extra standard extension tables are checked after the main standard extension table. This makes the common case of the main table matching occur earlier. -Zicfiss is the exception to this as it needs to be checked before the main table since it overrides some encodings from Zcmop. This can't be handled by a predicate based priority as Zicfiss only overrides a subset of Zcmop encodings.

llvmbot · 2025-02-26T22:49:44Z

@llvm/pr-subscribers-backend-risc-v

Author: Craig Topper (topperc)

Changes

First thing to know is that the subtarget feature checks used to block accessing a decoder table are only a performance optimization and not required for functionality. The tables have their own predicate checks. I've removed them from all the standard extension tables.

-RV32 Zacas decoder namespace has been renamed to RV32GPRPair, I think Zilsd(rv32 load/store pair) can go in here too. -The RV32 Zdinx table has been renamed to also use RV32GPRPair. -The Zfinx table has been renamed to remove superflous "RV" prefix. -Zcmp and Zcmp tables have been combined into a ZcOverlap table, I think
Zclsd(rv32 compressed load/store pair) can go in here too.
-All the extra standard extension tables are checked after the main
standard extension table. This makes the common case of the main table
matching occur earlier.
-Zicfiss is the exception to this as it needs to be checked before
the main table since it overrides some encodings from Zcmop. This
can't be handled by a predicate based priority as Zicfiss only overrides
a subset of Zcmop encodings.

Full diff: https://github.com/llvm/llvm-project/pull/128954.diff

6 Files Affected:

(modified) llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp (+13-19)
(modified) llvm/lib/Target/RISCV/RISCVInstrInfoD.td (+2-2)
(modified) llvm/lib/Target/RISCV/RISCVInstrInfoF.td (+1-1)
(modified) llvm/lib/Target/RISCV/RISCVInstrInfoZa.td (+1-1)
(modified) llvm/lib/Target/RISCV/RISCVInstrInfoZc.td (+6-6)
(modified) llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td (+6-6)

diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index 08e0cfee809bc..b790005cb3f27 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -657,16 +657,6 @@ DecodeStatus RISCVDisassembler::getInstruction32(MCInst &MI, uint64_t &Size,
 
   uint32_t Insn = support::endian::read32le(Bytes.data());
 
-  TRY_TO_DECODE(STI.hasFeature(RISCV::FeatureStdExtZdinx) &&
-                    !STI.hasFeature(RISCV::Feature64Bit),
-                DecoderTableRV32Zdinx32,
-                "RV32Zdinx (Double in Integer and rv32)");
-  TRY_TO_DECODE(STI.hasFeature(RISCV::FeatureStdExtZacas) &&
-                    !STI.hasFeature(RISCV::Feature64Bit),
-                DecoderTableRV32Zacas32,
-                "RV32Zacas (Compare-And-Swap and rv32)");
-  TRY_TO_DECODE_FEATURE(RISCV::FeatureStdExtZfinx, DecoderTableRVZfinx32,
-                        "RVZfinx (Float in Integer)");
   TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXVentanaCondOps,
                         DecoderTableXVentana32, "XVentanaCondOps");
   TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadBa, DecoderTableXTHeadBa32,
@@ -721,6 +711,11 @@ DecodeStatus RISCVDisassembler::getInstruction32(MCInst &MI, uint64_t &Size,
   TRY_TO_DECODE_FEATURE_ANY(XRivosFeatureGroup, DecoderTableXRivos32, "Rivos");
 
   TRY_TO_DECODE(true, DecoderTable32, "RISCV32");
+  TRY_TO_DECODE(true, DecoderTableRV32GPRPair32,
+                "RV32GPRPair (rv32 and GPR pairs)");
+  TRY_TO_DECODE(true, DecoderTableZfinx32, "Zfinx (Float in Integer)");
+  TRY_TO_DECODE(true, DecoderTableZdinxRV32GPRPair32,
+                "ZdinxRV32GPRPair (rv32 and Double in Integer)");
 
   return MCDisassembler::Fail;
 }
@@ -736,23 +731,22 @@ DecodeStatus RISCVDisassembler::getInstruction16(MCInst &MI, uint64_t &Size,
   Size = 2;
 
   uint32_t Insn = support::endian::read16le(Bytes.data());
-  TRY_TO_DECODE_AND_ADD_SP(!STI.hasFeature(RISCV::Feature64Bit),
-                           DecoderTableRISCV32Only_16,
-                           "RISCV32Only_16 (16-bit Instruction)");
-  TRY_TO_DECODE_FEATURE(RISCV::FeatureStdExtZicfiss, DecoderTableZicfiss16,
-                        "RVZicfiss (Shadow Stack)");
-  TRY_TO_DECODE_FEATURE(RISCV::FeatureStdExtZcmt, DecoderTableRVZcmt16,
-                        "Zcmt (16-bit Table Jump Instructions)");
-  TRY_TO_DECODE_FEATURE(RISCV::FeatureStdExtZcmp, DecoderTableRVZcmp16,
-                        "Zcmp (16-bit Push/Pop & Double Move Instructions)");
 
   TRY_TO_DECODE_FEATURE_ANY(XqciFeatureGroup, DecoderTableXqci16,
                             "Qualcomm uC 16bit");
 
   TRY_TO_DECODE_AND_ADD_SP(STI.hasFeature(RISCV::FeatureVendorXwchc),
                            DecoderTableXwchc16, "WCH QingKe XW");
+
+  // DecoderTableZicfiss16 must be checked before DecoderTable16.
+  TRY_TO_DECODE(true, DecoderTableZicfiss16, "RVZicfiss (Shadow Stack)");
   TRY_TO_DECODE_AND_ADD_SP(true, DecoderTable16,
                            "RISCV_C (16-bit Instruction)");
+  TRY_TO_DECODE_AND_ADD_SP(true, DecoderTableRISCV32Only_16,
+                           "RISCV32Only_16 (16-bit Instruction)");
+  // Zc* instructions incompatible with Zcf or Zcd.
+  TRY_TO_DECODE(true, DecoderTableZcOverlap16,
+                "ZcOverlap (16-bit Instructions overlapping with Zcf/Zcd)");
 
   return MCDisassembler::Fail;
 }
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoD.td b/llvm/lib/Target/RISCV/RISCVInstrInfoD.td
index 349bc361c90fe..89254940a87f4 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoD.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoD.td
@@ -60,9 +60,9 @@ def FPR64IN32X : RegisterOperand<GPRPair> {
 
 def DExt       : ExtInfo<"", "", [HasStdExtD], f64, FPR64, FPR32, FPR64, ?>;
 
-def ZdinxExt   : ExtInfo<"_INX", "RVZfinx", [HasStdExtZdinx, IsRV64],
+def ZdinxExt   : ExtInfo<"_INX", "Zfinx", [HasStdExtZdinx, IsRV64],
                          f64, FPR64INX, FPR32INX, FPR64INX, ?>;
-def Zdinx32Ext : ExtInfo<"_IN32X", "RV32Zdinx", [HasStdExtZdinx, IsRV32],
+def Zdinx32Ext : ExtInfo<"_IN32X", "ZdinxRV32GPRPair", [HasStdExtZdinx, IsRV32],
                          f64, FPR64IN32X, FPR32INX, FPR64IN32X, ?>;
 
 defvar DExts     = [DExt, ZdinxExt, Zdinx32Ext];
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoF.td b/llvm/lib/Target/RISCV/RISCVInstrInfoF.td
index 37ac48db06862..04328151adf8e 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoF.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoF.td
@@ -116,7 +116,7 @@ class ExtInfo<string suffix, string space, list<Predicate> predicates,
 
 def FExt       : ExtInfo<"", "", [HasStdExtF], f32, FPR32, FPR32, ?, ?>;
 
-def ZfinxExt   : ExtInfo<"_INX", "RVZfinx", [HasStdExtZfinx], f32, FPR32INX, FPR32INX, ?, ?>;
+def ZfinxExt   : ExtInfo<"_INX", "Zfinx", [HasStdExtZfinx], f32, FPR32INX, FPR32INX, ?, ?>;
 
 defvar FExts   = [FExt, ZfinxExt];
 
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZa.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZa.td
index 1ee78359bc4a5..e903df4d91933 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZa.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZa.td
@@ -59,7 +59,7 @@ let Predicates = [HasStdExtZacas], IsSignExtendingOpW = 1 in {
 defm AMOCAS_W : AMO_cas_aq_rl<0b00101, 0b010, "amocas.w", GPR>;
 } // Predicates = [HasStdExtZacas]
 
-let Predicates = [HasStdExtZacas, IsRV32], DecoderNamespace = "RV32Zacas"  in {
+let Predicates = [HasStdExtZacas, IsRV32], DecoderNamespace = "RV32GPRPair"  in {
 defm AMOCAS_D_RV32 : AMO_cas_aq_rl<0b00101, 0b011, "amocas.d", GPRPairRV32>;
 } // Predicates = [HasStdExtZacas, IsRV32]
 
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
index 1740ebb239217..b9cfd0dac6885 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
@@ -218,7 +218,7 @@ def C_SH_INX : CStoreH_rri<0b100011, 0b0, "c.sh", GPRF16C>,
 } // Predicates = [HasStdExtZcb]
 
 // Zcmp
-let DecoderNamespace = "RVZcmp", Predicates = [HasStdExtZcmp],
+let DecoderNamespace = "ZcOverlap", Predicates = [HasStdExtZcmp],
     hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
 let Defs = [X10, X11] in
 def CM_MVA01S : RVInst16CA<0b101011, 0b11, 0b10, (outs),
@@ -229,9 +229,9 @@ let Uses = [X10, X11] in
 def CM_MVSA01 : RVInst16CA<0b101011, 0b01, 0b10, (outs SR07:$rs1, SR07:$rs2),
                             (ins), "cm.mvsa01", "$rs1, $rs2">,
                 Sched<[WriteIALU, WriteIALU, ReadIALU, ReadIALU]>;
-} // DecoderNamespace = "RVZcmp", Predicates = [HasStdExtZcmp]...
+} // DecoderNamespace = "ZcOverlap", Predicates = [HasStdExtZcmp]...
 
-let DecoderNamespace = "RVZcmp", Predicates = [HasStdExtZcmp] in {
+let DecoderNamespace = "ZcOverlap", Predicates = [HasStdExtZcmp] in {
 let hasSideEffects = 0, mayLoad = 0, mayStore = 1, Uses = [X2], Defs = [X2] in
 def CM_PUSH : RVInstZcCPPP<0b11000, "cm.push", negstackadj>,
               Sched<[WriteIALU, ReadIALU, ReadStoreData, ReadStoreData,
@@ -260,9 +260,9 @@ def CM_POP : RVInstZcCPPP<0b11010, "cm.pop">,
              Sched<[WriteIALU, WriteLDW, WriteLDW, WriteLDW, WriteLDW,
                     WriteLDW, WriteLDW, WriteLDW, WriteLDW, WriteLDW, WriteLDW,
                     WriteLDW, WriteLDW, WriteLDW, ReadIALU]>;
-} // DecoderNamespace = "RVZcmp", Predicates = [HasStdExtZcmp]...
+} // DecoderNamespace = "ZcOverlap", Predicates = [HasStdExtZcmp]...
 
-let DecoderNamespace = "RVZcmt", Predicates = [HasStdExtZcmt],
+let DecoderNamespace = "ZcOverlap", Predicates = [HasStdExtZcmt],
     hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
 def CM_JT : RVInst16CJ<0b101, 0b10, (outs), (ins uimm5:$index),
                        "cm.jt", "$index">{
@@ -280,7 +280,7 @@ def CM_JALT : RVInst16CJ<0b101, 0b10, (outs), (ins uimm8ge32:$index),
   let Inst{12-10} = 0b000;
   let Inst{9-2} = index;
 }
-} // DecoderNamespace = "RVZcmt", Predicates = [HasStdExtZcmt]...
+} // DecoderNamespace = "ZcOverlap", Predicates = [HasStdExtZcmt]...
 
 
 let Predicates = [HasStdExtZcb, HasStdExtZmmul] in{
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td
index 625011c3b9f7c..ea0b814ac7ba5 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZfh.td
@@ -52,22 +52,22 @@ def ZfhDExt    : ExtInfo<"", "", [HasStdExtZfh, HasStdExtD],
 def ZfhminDExt : ExtInfo<"", "", [HasStdExtZfhmin, HasStdExtD],
                          ?, ?, FPR32, FPR64, FPR16>;
 
-def ZhinxExt            : ExtInfo<"_INX", "RVZfinx",
+def ZhinxExt            : ExtInfo<"_INX", "Zfinx",
                                   [HasStdExtZhinx],
                                   f16, FPR16INX, FPR32INX, ?, FPR16INX>;
-def ZhinxminExt         : ExtInfo<"_INX", "RVZfinx",
+def ZhinxminExt         : ExtInfo<"_INX", "Zfinx",
                                   [HasStdExtZhinxmin],
                                   f16, FPR16INX, FPR32INX, ?, FPR16INX>;
-def ZhinxZdinxExt       : ExtInfo<"_INX", "RVZfinx",
+def ZhinxZdinxExt       : ExtInfo<"_INX", "Zfinx",
                                   [HasStdExtZhinx, HasStdExtZdinx, IsRV64],
                                   ?, ?, FPR32INX, FPR64INX, FPR16INX>;
-def ZhinxminZdinxExt    : ExtInfo<"_INX", "RVZfinx",
+def ZhinxminZdinxExt    : ExtInfo<"_INX", "Zfinx",
                                   [HasStdExtZhinxmin, HasStdExtZdinx, IsRV64],
                                   ?, ?, FPR32INX, FPR64INX, FPR16INX>;
-def ZhinxZdinx32Ext     : ExtInfo<"_IN32X", "RV32Zdinx",
+def ZhinxZdinx32Ext     : ExtInfo<"_IN32X", "ZdinxGPRPairRV32",
                                   [HasStdExtZhinx, HasStdExtZdinx, IsRV32],
                                   ?, ?, FPR32INX, FPR64IN32X, FPR16INX >;
-def ZhinxminZdinx32Ext  : ExtInfo<"_IN32X", "RV32Zdinx",
+def ZhinxminZdinx32Ext  : ExtInfo<"_IN32X", "ZdinxGPRPairRV32",
                                   [HasStdExtZhinxmin, HasStdExtZdinx, IsRV32],
                                   ?, ?, FPR32INX, FPR64IN32X, FPR16INX>;

lenary

Nice, this looks neater, and is better for explaining when exactly the decoder predicate support is not good enough to avoid issues with conflicting encodings that we have in the backend.

I have one question, and I think there's a typo in the message where you mention Zcmp and Zcmp, one of which I think was supposed to be Zcmt

lenary · 2025-02-26T23:56:40Z

llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp

  TRY_TO_DECODE(true, DecoderTable32, "RISCV32");
+  TRY_TO_DECODE(true, DecoderTableRV32GPRPair32,
+                "RV32GPRPair (rv32 and GPR pairs)");
+  TRY_TO_DECODE(true, DecoderTableZfinx32, "Zfinx (Float in Integer)");
+  TRY_TO_DECODE(true, DecoderTableZdinxRV32GPRPair32,
+                "ZdinxRV32GPRPair (rv32 and Double in Integer)");


Why do we decode all the standard stuff last, rather than before custom extensions? I'd have thought we want to decode the standard extensions with higher priority than the custom extensions, but maybe not?

Why do we decode all the standard stuff last, rather than before custom extensions? I'd have thought we want to decode the standard extensions with higher priority than the custom extensions, but maybe not?

A good question. As far as I know all the vendor extensions are mutually exclusive with the standard extensions, except for Xqcisim which appears to use hint encodings of standard instructions. So they could go last and it would be faster for the common case.

I'm less concerned for speed, more for "we should prioritise decoding the standard extensions", which feels like a good principle. It sounds like doing so might require changing how we implemented Xqcisim, so I can do the reordering as a follow-up.

There were some concerns about ordering raised by @jrtc27 in https://reviews.llvm.org/D151309.

Downstream we have a separate encoding mode that replaces some standard instructions with alternative ones; even if the order doesn't matter upstream, it would be preferable for us if the standard table was the last one not the first one.

@jrtc27 why couldn't you put your downstream stuff before the standard table even if the standard table is first upstream?

topperc · 2025-02-27T22:23:54Z

Can this patch go in as is and let the ordering issue be resolved separately?

lenary · 2025-02-27T22:54:01Z

Can this patch go in as is and let the ordering issue be resolved separately?

Yes. ~~Did you note my comment about the Zcmp/Zcmt typo?~~ edit: Yes you did

…lvm#128954) First thing to know is that the subtarget feature checks used to block accessing a decoder table are only a performance optimization and not required for functionality. The tables have their own predicate checks. I've removed them from all the standard extension tables. -RV32 Zacas decoder namespace has been renamed to RV32GPRPair, I think Zilsd(rv32 load/store pair) can go in here too. -The RV32 Zdinx table has been renamed to also use RV32GPRPair. -The Zfinx table has been renamed to remove superflous "RV" prefix. -Zcmp and Zcmt tables have been combined into a ZcOverlap table. I think Zclsd(rv32 compressed load/store pair) can go in here too. -All the extra standard extension tables are checked after the main standard extension table. This makes the common case of the main table matching occur earlier. -Zicfiss is the exception to this as it needs to be checked before the main table since it overrides some encodings from Zcmop. This can't be handled by a predicate based priority as Zicfiss only overrides a subset of Zcmop encodings.

topperc requested review from asb, lenary, preames and wangpc-pp February 26, 2025 22:49

llvmbot added the backend:RISC-V label Feb 26, 2025

lenary reviewed Feb 26, 2025

View reviewed changes

lenary approved these changes Feb 27, 2025

View reviewed changes

topperc merged commit f3b1849 into llvm:main Feb 27, 2025
13 checks passed

topperc deleted the pr/decode-consolidation branch February 27, 2025 22:56

huaatian mentioned this pull request Feb 28, 2025

fix live interval empty issue huaatian/llvm-project#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[RISCV] Consolidate some DecoderNamespaces for standard extensions. #128954

[RISCV] Consolidate some DecoderNamespaces for standard extensions. #128954

Uh oh!

topperc commented Feb 26, 2025 •

edited

Loading

Uh oh!

llvmbot commented Feb 26, 2025

Uh oh!

lenary left a comment

Uh oh!

lenary Feb 26, 2025

Uh oh!

topperc Feb 27, 2025

Uh oh!

lenary Feb 27, 2025

Uh oh!

wangpc-pp Feb 27, 2025

Uh oh!

topperc Feb 27, 2025

Uh oh!

topperc commented Feb 27, 2025

Uh oh!

lenary commented Feb 27, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

[RISCV] Consolidate some DecoderNamespaces for standard extensions. #128954

[RISCV] Consolidate some DecoderNamespaces for standard extensions. #128954

Uh oh!

Conversation

topperc commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Feb 26, 2025

Uh oh!

lenary left a comment

Choose a reason for hiding this comment

Uh oh!

lenary Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

topperc Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

lenary Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

wangpc-pp Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

topperc Feb 27, 2025

Choose a reason for hiding this comment

Uh oh!

topperc commented Feb 27, 2025

Uh oh!

lenary commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

topperc commented Feb 26, 2025 •

edited

Loading

lenary commented Feb 27, 2025 •

edited

Loading