[llvm] [RISCV][Xqccmp] Correctly Parse/Disassemble pushfp (PR #133188)

Sam Elliott via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 27 11:21:12 PDT 2025


https://github.com/lenary updated https://github.com/llvm/llvm-project/pull/133188

>From 8ab597e7ff76a13ab9d32a551f5fbc9377ca231e Mon Sep 17 00:00:00 2001
From: Sam Elliott <quic_aelliott at quicinc.com>
Date: Wed, 26 Mar 2025 17:59:49 -0700
Subject: [PATCH 1/8] [RISCV][Xqccmp] Correctly Parse/Disassemble pushfp

In the `qc.cm.pushfp` instruction, it is like `cm.pushfp` except in one
important way - `qc.cm.pushfp {ra}, -N*16` is not a valid encoding,
because this would update `s0`/`fp`/`x8` without saving it.

This change now correctly rejects this variant of the instruction, both
during parsing and during disassembly. I also implemented validation for
immediates that represent register lists (both kinds), which may help to
catch bugs in the future.
---
 .../Target/RISCV/AsmParser/RISCVAsmParser.cpp | 27 ++++++++++++-
 .../RISCV/Disassembler/RISCVDisassembler.cpp  | 11 +++++
 .../Target/RISCV/MCTargetDesc/RISCVBaseInfo.h |  2 +
 .../RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp | 15 +++++++
 llvm/lib/Target/RISCV/RISCVInstrInfo.cpp      |  6 +++
 llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td | 40 ++++++++++++++++++-
 llvm/lib/Target/RISCV/RISCVInstrInfoZc.td     |  4 +-
 .../RISCV/xqccmp-invalid-rlist.txt            | 11 +++++
 llvm/test/MC/RISCV/rv32xqccmp-invalid.s       |  4 ++
 llvm/test/MC/RISCV/rv32xqccmp-valid.s         |  8 ----
 llvm/test/MC/RISCV/rv64e-xqccmp-valid.s       |  4 --
 llvm/test/MC/RISCV/rv64xqccmp-valid.s         |  4 --
 12 files changed, 116 insertions(+), 20 deletions(-)
 create mode 100644 llvm/test/MC/Disassembler/RISCV/xqccmp-invalid-rlist.txt

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 0d40fb5614009..19f28a6287060 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -215,7 +215,10 @@ class RISCVAsmParser : public MCTargetAsmParser {
   ParseStatus parseGPRPair(OperandVector &Operands, bool IsRV64Inst);
   ParseStatus parseFRMArg(OperandVector &Operands);
   ParseStatus parseFenceArg(OperandVector &Operands);
+
+  template <bool MustIncludeS0 = false>
   ParseStatus parseReglist(OperandVector &Operands);
+
   ParseStatus parseRegReg(OperandVector &Operands);
   ParseStatus parseRetval(OperandVector &Operands);
   ParseStatus parseZcmpStackAdj(OperandVector &Operands,
@@ -474,6 +477,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
   bool isSystemRegister() const { return Kind == KindTy::SystemRegister; }
   bool isRegReg() const { return Kind == KindTy::RegReg; }
   bool isRlist() const { return Kind == KindTy::Rlist; }
+  bool isRlistS0() const { return Kind == KindTy::Rlist && Rlist.Val != 4; }
   bool isSpimm() const { return Kind == KindTy::Spimm; }
 
   bool isGPR() const {
@@ -2794,9 +2798,15 @@ ParseStatus RISCVAsmParser::parseRegReg(OperandVector &Operands) {
   return ParseStatus::Success;
 }
 
+template <bool MustIncludeS0>
 ParseStatus RISCVAsmParser::parseReglist(OperandVector &Operands) {
   // Rlist: {ra [, s0[-sN]]}
   // XRlist: {x1 [, x8[-x9][, x18[-xN]]]}
+
+  // When MustIncludeS0 = true (not the default) (used for `qc.cm.pushfp`) which
+  // must include `fp`/`s0` in the list:
+  // Rlist: {ra, s0[-sN]}
+  // XRlist: {x1, x8[-x9][, x18[-xN]]}
   SMLoc S = getLoc();
 
   if (parseToken(AsmToken::LCurly, "register list must start with '{'"))
@@ -2814,8 +2824,19 @@ ParseStatus RISCVAsmParser::parseReglist(OperandVector &Operands) {
     return Error(getLoc(), "register list must start from 'ra' or 'x1'");
   getLexer().Lex();
 
-  // parse case like ,s0
-  if (parseOptionalToken(AsmToken::Comma)) {
+  bool SeenComma = false;
+
+  // If you must include S0, then you need the comma. Fail now if it isn't there
+  if constexpr (MustIncludeS0) {
+    if (parseToken(AsmToken::Comma, "register list must include 's0' or 'x8'"))
+      return ParseStatus::Failure;
+    SeenComma = true;
+  } else {
+    SeenComma = parseOptionalToken(AsmToken::Comma);
+  }
+
+  // parse case like ,s0 - we may have already seen the comma in which case skip
+  if (SeenComma) {
     if (getLexer().isNot(AsmToken::Identifier))
       return Error(getLoc(), "invalid register");
     StringRef RegName = getLexer().getTok().getIdentifier();
@@ -2884,6 +2905,8 @@ ParseStatus RISCVAsmParser::parseReglist(OperandVector &Operands) {
 
   auto Encode = RISCVZC::encodeRlist(RegEnd, IsEABI);
   assert(Encode != RISCVZC::INVALID_RLIST);
+  if constexpr (MustIncludeS0)
+    assert(Encode != RISCVZC::RA);
   Operands.push_back(RISCVOperand::createRlist(Encode, S));
 
   return ParseStatus::Success;
diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index 93cbf662bfa32..d03351db5473b 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -507,6 +507,9 @@ static DecodeStatus decodeXTHeadMemPair(MCInst &Inst, uint32_t Insn,
 static DecodeStatus decodeZcmpRlist(MCInst &Inst, uint32_t Imm,
                                     uint64_t Address, const void *Decoder);
 
+static DecodeStatus decodeXqccmpRlistS0(MCInst &Inst, uint32_t Imm,
+                                        uint64_t Address, const void *Decoder);
+
 static DecodeStatus decodeRegReg(MCInst &Inst, uint32_t Insn, uint64_t Address,
                                  const MCDisassembler *Decoder);
 
@@ -621,6 +624,14 @@ static DecodeStatus decodeZcmpRlist(MCInst &Inst, uint32_t Imm,
   return MCDisassembler::Success;
 }
 
+static DecodeStatus decodeXqccmpRlistS0(MCInst &Inst, uint32_t Imm,
+                                        uint64_t Address, const void *Decoder) {
+  if (Imm <= 4)
+    return MCDisassembler::Fail;
+  Inst.addOperand(MCOperand::createImm(Imm));
+  return MCDisassembler::Success;
+}
+
 static DecodeStatus decodeRegReg(MCInst &Inst, uint32_t Insn, uint64_t Address,
                                  const MCDisassembler *Decoder) {
   uint32_t Rs1 = fieldFromInstruction(Insn, 0, 5);
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
index db305b0083415..0ede9ba7cf712 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h
@@ -343,6 +343,8 @@ enum OperandType : unsigned {
   OPERAND_RVKRNUM_0_7,
   OPERAND_RVKRNUM_1_10,
   OPERAND_RVKRNUM_2_14,
+  OPERAND_RLIST,
+  OPERAND_RLIST_S0,
   OPERAND_SPIMM,
   // Operand is a 3-bit rounding mode, '111' indicates FRM register.
   // Represents 'frm' argument passing to floating-point operations.
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
index e589e6171d010..6163175347ca0 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
@@ -104,6 +104,10 @@ class RISCVMCCodeEmitter : public MCCodeEmitter {
                            SmallVectorImpl<MCFixup> &Fixups,
                            const MCSubtargetInfo &STI) const;
 
+  unsigned getRlistS0OpValue(const MCInst &MI, unsigned OpNo,
+                             SmallVectorImpl<MCFixup> &Fixups,
+                             const MCSubtargetInfo &STI) const;
+
   unsigned getRegReg(const MCInst &MI, unsigned OpNo,
                      SmallVectorImpl<MCFixup> &Fixups,
                      const MCSubtargetInfo &STI) const;
@@ -621,6 +625,17 @@ unsigned RISCVMCCodeEmitter::getRlistOpValue(const MCInst &MI, unsigned OpNo,
   return Imm;
 }
 
+unsigned
+RISCVMCCodeEmitter::getRlistS0OpValue(const MCInst &MI, unsigned OpNo,
+                                      SmallVectorImpl<MCFixup> &Fixups,
+                                      const MCSubtargetInfo &STI) const {
+  const MCOperand &MO = MI.getOperand(OpNo);
+  assert(MO.isImm() && "Rlist operand must be immediate");
+  auto Imm = MO.getImm();
+  assert(Imm >= 5 && "EABI is currently not implemented");
+  return Imm;
+}
+
 unsigned RISCVMCCodeEmitter::getRegReg(const MCInst &MI, unsigned OpNo,
                                        SmallVectorImpl<MCFixup> &Fixups,
                                        const MCSubtargetInfo &STI) const {
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
index 4fb11f278df97..7867fd7d516cc 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.cpp
@@ -2642,6 +2642,12 @@ bool RISCVInstrInfo::verifyInstruction(const MachineInstr &MI,
         case RISCVOp::OPERAND_RVKRNUM_2_14:
           Ok = Imm >= 2 && Imm <= 14;
           break;
+        case RISCVOp::OPERAND_RLIST:
+          Ok = Imm >= RISCVZC::RA && Imm <= RISCVZC::RA_S0_S11;
+          break;
+        case RISCVOp::OPERAND_RLIST_S0:
+          Ok = Imm >= RISCVZC::RA_S0 && Imm <= RISCVZC::RA_S0_S11;
+          break;
         case RISCVOp::OPERAND_SPIMM:
           Ok = (Imm & 0xf) == 0;
           break;
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td
index 5bb9c1e4b228b..8cb1cf151f1ad 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td
@@ -25,6 +25,30 @@
 // Operand and SDNode transformation definitions.
 //===----------------------------------------------------------------------===//
 
+def RlistS0AsmOperand : AsmOperandClass {
+  let Name = "RlistS0";
+  let ParserMethod = "parseReglist<true>";
+  let RenderMethod = "addRlistOperands";
+  let DiagnosticType = "InvalidRlistS0";
+  let DiagnosticString = "operand must be {ra, s0[-sN]} or {x1, x8[-x9][, x18[-xN]]}";
+}
+
+def rlist_s0 : RISCVOp<OtherVT> {
+   let ParserMatchClass = RlistS0AsmOperand;
+   let PrintMethod = "printRlist";
+   let DecoderMethod = "decodeXqccmpRlistS0";
+   let EncoderMethod = "getRlistS0OpValue";
+   let MCOperandPredicate = [{
+    int64_t Imm;
+    if (!MCOp.evaluateAsConstantImm(Imm))
+      return false;
+    // 0~4 invalid for `qc.cm.pushfp`
+    return isUInt<4>(Imm) && Imm >= 5;
+  }];
+
+  string OperandType = "OPERAND_RLIST_S0";
+}
+
 //===----------------------------------------------------------------------===//
 // Instruction Formats
 //===----------------------------------------------------------------------===//
@@ -33,6 +57,20 @@
 // Instruction Class Templates
 //===----------------------------------------------------------------------===//
 
+class RVInstXqccmpCPPPFP<bits<5> funct5, string opcodestr,
+                         DAGOperand immtype = stackadj>
+    : RVInst16<(outs), (ins rlist_s0:$rlist, immtype:$stackadj),
+               opcodestr, "$rlist, $stackadj", [], InstFormatOther> {
+  bits<4> rlist;
+  bits<16> stackadj;
+
+  let Inst{1-0} = 0b10;
+  let Inst{3-2} = stackadj{5-4};
+  let Inst{7-4} = rlist;
+  let Inst{12-8} = funct5;
+  let Inst{15-13} = 0b101;
+}
+
 //===----------------------------------------------------------------------===//
 // Instructions
 //===----------------------------------------------------------------------===//
@@ -59,7 +97,7 @@ def QC_CM_PUSH : RVInstZcCPPP<0b11000, "qc.cm.push", negstackadj>,
                         ReadStoreData, ReadStoreData, ReadStoreData]>;
 
 let hasSideEffects = 0, mayLoad = 0, mayStore = 1, Uses = [X2], Defs = [X2, X8] in
-def QC_CM_PUSHFP : RVInstZcCPPP<0b11001, "qc.cm.pushfp", negstackadj>,
+def QC_CM_PUSHFP : RVInstXqccmpCPPPFP<0b11001, "qc.cm.pushfp", negstackadj>,
                    Sched<[WriteIALU, WriteIALU, ReadIALU, ReadStoreData, ReadStoreData,
                           ReadStoreData, ReadStoreData, ReadStoreData, ReadStoreData,
                           ReadStoreData, ReadStoreData, ReadStoreData, ReadStoreData,
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
index efed74ca8c870..04c10220f5bf1 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
@@ -58,7 +58,7 @@ def NegStackAdjAsmOperand : AsmOperandClass {
   let RenderMethod = "addSpimmOperands";
 }
 
-def rlist : Operand<OtherVT> {
+def rlist : RISCVOp<OtherVT> {
    let ParserMatchClass = RlistAsmOperand;
    let PrintMethod = "printRlist";
    let DecoderMethod = "decodeZcmpRlist";
@@ -70,6 +70,8 @@ def rlist : Operand<OtherVT> {
     // 0~3 Reserved for EABI
     return isUInt<4>(Imm) && Imm >= 4;
   }];
+
+  let OperandType = "OPERAND_RLIST";
 }
 
 def stackadj : RISCVOp<OtherVT> {
diff --git a/llvm/test/MC/Disassembler/RISCV/xqccmp-invalid-rlist.txt b/llvm/test/MC/Disassembler/RISCV/xqccmp-invalid-rlist.txt
new file mode 100644
index 0000000000000..fe81c01a037af
--- /dev/null
+++ b/llvm/test/MC/Disassembler/RISCV/xqccmp-invalid-rlist.txt
@@ -0,0 +1,11 @@
+# RUN: not llvm-mc -disassemble -triple=riscv32 -mattr=+experimental-xqccmp %s \
+# RUN:   | FileCheck -check-prefixes=CHECK,CHECK-XQCCMP %s
+
+[0x00,0x00]
+# CHECK: unimp
+
+[0x42,0xb9]
+# CHECK-XQCCMP-NOT: qc.cm.pushfp {ra}, -{{[0-9]+}}
+
+[0x00,0x00]
+# CHECK: unimp
diff --git a/llvm/test/MC/RISCV/rv32xqccmp-invalid.s b/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
index 74f96f076756c..059009de3d830 100644
--- a/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
+++ b/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
@@ -33,3 +33,7 @@ qc.cm.pushfp {ra, s0}, -12
 
 # CHECK-ERROR: error: stack adjustment for register list must be a multiple of 16 bytes in the range [16, 64]
 qc.cm.pop {ra, s0-s1}, -40
+
+# CHECK-ERROR: error: register list must include 's0' or 'x8'
+qc.cm.pushfp {ra}, -16
+
diff --git a/llvm/test/MC/RISCV/rv32xqccmp-valid.s b/llvm/test/MC/RISCV/rv32xqccmp-valid.s
index 5827777e524ca..b1f373ede247d 100644
--- a/llvm/test/MC/RISCV/rv32xqccmp-valid.s
+++ b/llvm/test/MC/RISCV/rv32xqccmp-valid.s
@@ -288,14 +288,6 @@ qc.cm.push {ra, s0-s11}, -112
 # CHECK-ASM: encoding: [0xfe,0xb8]
 qc.cm.push {x1, x8-x9, x18-x27}, -112
 
-# CHECK-ASM-AND-OBJ: qc.cm.pushfp {ra}, -16
-# CHECK-ASM: encoding: [0x42,0xb9]
-qc.cm.pushfp {ra}, -16
-
-# CHECK-ASM-AND-OBJ: qc.cm.pushfp {ra}, -16
-# CHECK-ASM: encoding: [0x42,0xb9]
-qc.cm.pushfp {x1}, -16
-
 # CHECK-ASM-AND-OBJ: qc.cm.pushfp {ra, s0}, -32
 # CHECK-ASM: encoding: [0x56,0xb9]
 qc.cm.pushfp {ra, s0}, -32
diff --git a/llvm/test/MC/RISCV/rv64e-xqccmp-valid.s b/llvm/test/MC/RISCV/rv64e-xqccmp-valid.s
index 8f9e3ce7ee533..003f9852b184a 100644
--- a/llvm/test/MC/RISCV/rv64e-xqccmp-valid.s
+++ b/llvm/test/MC/RISCV/rv64e-xqccmp-valid.s
@@ -72,10 +72,6 @@ qc.cm.push {ra, s0}, -32
 # CHECK-ASM: encoding: [0x62,0xb8]
 qc.cm.push {ra, s0-s1}, -32
 
-# CHECK-ASM-AND-OBJ: qc.cm.pushfp {ra}, -16
-# CHECK-ASM: encoding: [0x42,0xb9]
-qc.cm.pushfp {ra}, -16
-
 # CHECK-ASM-AND-OBJ: qc.cm.pushfp {ra, s0}, -32
 # CHECK-ASM: encoding: [0x56,0xb9]
 qc.cm.pushfp {ra, s0}, -32
diff --git a/llvm/test/MC/RISCV/rv64xqccmp-valid.s b/llvm/test/MC/RISCV/rv64xqccmp-valid.s
index 06ba33fe8a495..29cf6f0dfbdac 100644
--- a/llvm/test/MC/RISCV/rv64xqccmp-valid.s
+++ b/llvm/test/MC/RISCV/rv64xqccmp-valid.s
@@ -148,10 +148,6 @@ qc.cm.push {ra, s0-s11}, -112
 # CHECK-ASM: encoding: [0xf6,0xb8]
 qc.cm.push {ra, s0-s11}, -128
 
-# CHECK-ASM-AND-OBJ: qc.cm.pushfp {ra}, -16
-# CHECK-ASM: encoding: [0x42,0xb9]
-qc.cm.pushfp {ra}, -16
-
 # CHECK-ASM-AND-OBJ: qc.cm.pushfp {ra, s0}, -32
 # CHECK-ASM: encoding: [0x56,0xb9]
 qc.cm.pushfp {ra, s0}, -32

>From e7b66e931023845488a4f52a6b5f0aa9614f5951 Mon Sep 17 00:00:00 2001
From: Sam Elliott <quic_aelliott at quicinc.com>
Date: Wed, 26 Mar 2025 18:09:31 -0700
Subject: [PATCH 2/8] Clarify Comment

---
 llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 19f28a6287060..fe05929d85f17 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -2826,7 +2826,11 @@ ParseStatus RISCVAsmParser::parseReglist(OperandVector &Operands) {
 
   bool SeenComma = false;
 
-  // If you must include S0, then you need the comma. Fail now if it isn't there
+  // There are two choices here:
+  // - `s0` is required (qc.cm.pushfp), and so we must see the comma between
+  //   `ra` and `s0` and must always try to parse `s0`, below
+  // - `s0` is not required (usual case), so only try to parse `s0` if there is
+  //   a comma
   if constexpr (MustIncludeS0) {
     if (parseToken(AsmToken::Comma, "register list must include 's0' or 'x8'"))
       return ParseStatus::Failure;
@@ -2835,7 +2839,7 @@ ParseStatus RISCVAsmParser::parseReglist(OperandVector &Operands) {
     SeenComma = parseOptionalToken(AsmToken::Comma);
   }
 
-  // parse case like ,s0 - we may have already seen the comma in which case skip
+  // parse case like , s0 (knowing the comma is or must be there)
   if (SeenComma) {
     if (getLexer().isNot(AsmToken::Identifier))
       return Error(getLoc(), "invalid register");

>From df0858a249217663738bf80cb236502c1252c466 Mon Sep 17 00:00:00 2001
From: Sam Elliott <quic_aelliott at quicinc.com>
Date: Wed, 26 Mar 2025 18:50:14 -0700
Subject: [PATCH 3/8] Use Named Constants

---
 llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp   | 2 +-
 llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index fe05929d85f17..0fc306ca8a258 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -477,7 +477,7 @@ struct RISCVOperand final : public MCParsedAsmOperand {
   bool isSystemRegister() const { return Kind == KindTy::SystemRegister; }
   bool isRegReg() const { return Kind == KindTy::RegReg; }
   bool isRlist() const { return Kind == KindTy::Rlist; }
-  bool isRlistS0() const { return Kind == KindTy::Rlist && Rlist.Val != 4; }
+  bool isRlistS0() const { return Kind == KindTy::Rlist && Rlist.Val != RISCVZC::RA; }
   bool isSpimm() const { return Kind == KindTy::Spimm; }
 
   bool isGPR() const {
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
index 1829291cd0348..5391c522d589d 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
@@ -242,11 +242,11 @@ float RISCVLoadFPImm::getFPImm(unsigned Imm) {
 
 void RISCVZC::printRlist(unsigned SlistEncode, raw_ostream &OS) {
   OS << "{ra";
-  if (SlistEncode > 4) {
+  if (SlistEncode > RISCVZC::RA) {
     OS << ", s0";
-    if (SlistEncode == 15)
+    if (SlistEncode == RISCVZC::RA_S0_S11)
       OS << "-s11";
-    else if (SlistEncode > 5 && SlistEncode <= 14)
+    else if (SlistEncode > RISCVZC::RA_S0 && SlistEncode <= RISCVZC::RA_S0_S11)
       OS << "-s" << (SlistEncode - 5);
   }
   OS << "}";

>From e2a3edce4385c2cbb00464e3878323213c12ad02 Mon Sep 17 00:00:00 2001
From: Sam Elliott <quic_aelliott at quicinc.com>
Date: Wed, 26 Mar 2025 18:57:38 -0700
Subject: [PATCH 4/8] Address craig's feedback

---
 .../Target/RISCV/AsmParser/RISCVAsmParser.cpp | 32 +++++++++----------
 .../RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp |  3 +-
 llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td |  2 +-
 3 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 0fc306ca8a258..5f09ca70aaec6 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -215,9 +215,13 @@ class RISCVAsmParser : public MCTargetAsmParser {
   ParseStatus parseGPRPair(OperandVector &Operands, bool IsRV64Inst);
   ParseStatus parseFRMArg(OperandVector &Operands);
   ParseStatus parseFenceArg(OperandVector &Operands);
-
-  template <bool MustIncludeS0 = false>
-  ParseStatus parseReglist(OperandVector &Operands);
+  ParseStatus parseReglist(OperandVector &Operands) {
+    return parseRegListCommon(Operands, /*MustIncludeS0=*/false);
+  }
+  ParseStatus parseReglistS0(OperandVector &Operands) {
+    return parseRegListCommon(Operands, /*MustIncludeS0=*/true);
+  }
+  ParseStatus parseRegListCommon(OperandVector &Operands, bool MustIncludeS0);
 
   ParseStatus parseRegReg(OperandVector &Operands);
   ParseStatus parseRetval(OperandVector &Operands);
@@ -2798,8 +2802,7 @@ ParseStatus RISCVAsmParser::parseRegReg(OperandVector &Operands) {
   return ParseStatus::Success;
 }
 
-template <bool MustIncludeS0>
-ParseStatus RISCVAsmParser::parseReglist(OperandVector &Operands) {
+ParseStatus RISCVAsmParser::parseRegListCommon(OperandVector &Operands, bool MustIncludeS0) {
   // Rlist: {ra [, s0[-sN]]}
   // XRlist: {x1 [, x8[-x9][, x18[-xN]]]}
 
@@ -2824,22 +2827,19 @@ ParseStatus RISCVAsmParser::parseReglist(OperandVector &Operands) {
     return Error(getLoc(), "register list must start from 'ra' or 'x1'");
   getLexer().Lex();
 
-  bool SeenComma = false;
+  bool SeenComma = parseOptionalToken(AsmToken::Comma);
 
   // There are two choices here:
-  // - `s0` is required (qc.cm.pushfp), and so we must see the comma between
-  //   `ra` and `s0` and must always try to parse `s0`, below
   // - `s0` is not required (usual case), so only try to parse `s0` if there is
   //   a comma
-  if constexpr (MustIncludeS0) {
-    if (parseToken(AsmToken::Comma, "register list must include 's0' or 'x8'"))
-      return ParseStatus::Failure;
-    SeenComma = true;
-  } else {
-    SeenComma = parseOptionalToken(AsmToken::Comma);
+  // - `s0` is required (qc.cm.pushfp), and so we must see the comma between
+  //   `ra` and `s0` and must always try to parse `s0`, below
+  if (MustIncludeS0 && !SeenComma) {
+    Error(getLoc(), "register list must include 's0' or 'x8'");
+    return ParseStatus::Failure;
   }
 
-  // parse case like , s0 (knowing the comma is or must be there)
+  // parse case like ,s0 (knowing the comma must be there if required)
   if (SeenComma) {
     if (getLexer().isNot(AsmToken::Identifier))
       return Error(getLoc(), "invalid register");
@@ -2909,7 +2909,7 @@ ParseStatus RISCVAsmParser::parseReglist(OperandVector &Operands) {
 
   auto Encode = RISCVZC::encodeRlist(RegEnd, IsEABI);
   assert(Encode != RISCVZC::INVALID_RLIST);
-  if constexpr (MustIncludeS0)
+  if (MustIncludeS0)
     assert(Encode != RISCVZC::RA);
   Operands.push_back(RISCVOperand::createRlist(Encode, S));
 
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
index 6163175347ca0..6c7917b5d8b16 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
@@ -632,7 +632,8 @@ RISCVMCCodeEmitter::getRlistS0OpValue(const MCInst &MI, unsigned OpNo,
   const MCOperand &MO = MI.getOperand(OpNo);
   assert(MO.isImm() && "Rlist operand must be immediate");
   auto Imm = MO.getImm();
-  assert(Imm >= 5 && "EABI is currently not implemented");
+  assert(Imm >= 4 && "EABI is currently not implemented");
+  assert(Imm != RISCVZC::RA && "Rlist operand must include s0");
   return Imm;
 }
 
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td
index 8cb1cf151f1ad..82f15d7bdbf9a 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td
@@ -27,7 +27,7 @@
 
 def RlistS0AsmOperand : AsmOperandClass {
   let Name = "RlistS0";
-  let ParserMethod = "parseReglist<true>";
+  let ParserMethod = "parseReglistS0";
   let RenderMethod = "addRlistOperands";
   let DiagnosticType = "InvalidRlistS0";
   let DiagnosticString = "operand must be {ra, s0[-sN]} or {x1, x8[-x9][, x18[-xN]]}";

>From 8ed19efa57a1b03e1634ec613c4fd8b7982e0eb4 Mon Sep 17 00:00:00 2001
From: Sam Elliott <quic_aelliott at quicinc.com>
Date: Wed, 26 Mar 2025 19:01:14 -0700
Subject: [PATCH 5/8] More named constants

---
 llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td | 2 +-
 llvm/lib/Target/RISCV/RISCVInstrInfoZc.td     | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td b/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td
index 82f15d7bdbf9a..bee937f91f46c 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoXqccmp.td
@@ -43,7 +43,7 @@ def rlist_s0 : RISCVOp<OtherVT> {
     if (!MCOp.evaluateAsConstantImm(Imm))
       return false;
     // 0~4 invalid for `qc.cm.pushfp`
-    return isUInt<4>(Imm) && Imm >= 5;
+    return isUInt<4>(Imm) && Imm >= RISCVZC::RA_S0;
   }];
 
   string OperandType = "OPERAND_RLIST_S0";
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
index 04c10220f5bf1..3f90714cdbe88 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZc.td
@@ -68,7 +68,7 @@ def rlist : RISCVOp<OtherVT> {
     if (!MCOp.evaluateAsConstantImm(Imm))
       return false;
     // 0~3 Reserved for EABI
-    return isUInt<4>(Imm) && Imm >= 4;
+    return isUInt<4>(Imm) && Imm >= RISCVZC::RA;
   }];
 
   let OperandType = "OPERAND_RLIST";

>From 8cb2ecf7f2177384869c29ea1cbdccb675b6fa4d Mon Sep 17 00:00:00 2001
From: Sam Elliott <quic_aelliott at quicinc.com>
Date: Wed, 26 Mar 2025 19:01:38 -0700
Subject: [PATCH 6/8] clang-format

---
 llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 5f09ca70aaec6..f576db18353a2 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -481,7 +481,9 @@ struct RISCVOperand final : public MCParsedAsmOperand {
   bool isSystemRegister() const { return Kind == KindTy::SystemRegister; }
   bool isRegReg() const { return Kind == KindTy::RegReg; }
   bool isRlist() const { return Kind == KindTy::Rlist; }
-  bool isRlistS0() const { return Kind == KindTy::Rlist && Rlist.Val != RISCVZC::RA; }
+  bool isRlistS0() const {
+    return Kind == KindTy::Rlist && Rlist.Val != RISCVZC::RA;
+  }
   bool isSpimm() const { return Kind == KindTy::Spimm; }
 
   bool isGPR() const {
@@ -2802,7 +2804,8 @@ ParseStatus RISCVAsmParser::parseRegReg(OperandVector &Operands) {
   return ParseStatus::Success;
 }
 
-ParseStatus RISCVAsmParser::parseRegListCommon(OperandVector &Operands, bool MustIncludeS0) {
+ParseStatus RISCVAsmParser::parseRegListCommon(OperandVector &Operands,
+                                               bool MustIncludeS0) {
   // Rlist: {ra [, s0[-sN]]}
   // XRlist: {x1 [, x8[-x9][, x18[-xN]]]}
 

>From c188fe2630197682cd662759c47d493a3e7a978d Mon Sep 17 00:00:00 2001
From: Sam Elliott <quic_aelliott at quicinc.com>
Date: Thu, 27 Mar 2025 10:57:16 -0700
Subject: [PATCH 7/8] More named constants

---
 llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
index 5391c522d589d..d5f08ac05f82b 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp
@@ -247,7 +247,7 @@ void RISCVZC::printRlist(unsigned SlistEncode, raw_ostream &OS) {
     if (SlistEncode == RISCVZC::RA_S0_S11)
       OS << "-s11";
     else if (SlistEncode > RISCVZC::RA_S0 && SlistEncode <= RISCVZC::RA_S0_S11)
-      OS << "-s" << (SlistEncode - 5);
+      OS << "-s" << (SlistEncode - RISCVZC::RA_S0);
   }
   OS << "}";
 }

>From b937498d92f55264623128559b68316a5d10cd3f Mon Sep 17 00:00:00 2001
From: Sam Elliott <quic_aelliott at quicinc.com>
Date: Thu, 27 Mar 2025 11:20:51 -0700
Subject: [PATCH 8/8] More constants

---
 llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index 0edde205cb5b3..79647312393c7 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -616,7 +616,7 @@ static DecodeStatus decodeXTHeadMemPair(MCInst &Inst, uint32_t Insn,
 
 static DecodeStatus decodeZcmpRlist(MCInst &Inst, uint32_t Imm,
                                     uint64_t Address, const void *Decoder) {
-  if (Imm <= 3)
+  if (Imm < RISCVZC::RA)
     return MCDisassembler::Fail;
   Inst.addOperand(MCOperand::createImm(Imm));
   return MCDisassembler::Success;
@@ -624,7 +624,7 @@ static DecodeStatus decodeZcmpRlist(MCInst &Inst, uint32_t Imm,
 
 static DecodeStatus decodeXqccmpRlistS0(MCInst &Inst, uint32_t Imm,
                                         uint64_t Address, const void *Decoder) {
-  if (Imm <= 4)
+  if (Imm < RISCVZC::RA_S0)
     return MCDisassembler::Fail;
   Inst.addOperand(MCOperand::createImm(Imm));
   return MCDisassembler::Success;



More information about the llvm-commits mailing list