[RISCV] Check S0 register list check for qc.cm.pushfp to after we parsed the whole register list.

[topperc]

In my mind, this is more of a semantic check. I've also changed the diagnostic location to point at the register list start instead of the closing brace, or whatever character might be there instead of a brace if its malformed.

[llvmbot]

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

Author: Craig Topper (topperc)

Changes

In my mind, this is more of a semantic check. I've also changed the diagnostic location to point at the register list start instead of the closing brace, or whatever character might be there instead of a brace if its malformed.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp (+5-15)
• (modified) llvm/test/MC/RISCV/rv32xqccmp-invalid.s (+1-1)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 7837504751694..ed890104d488d 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -2588,20 +2588,8 @@ ParseStatus RISCVAsmParser::parseRegListCommon(OperandVector &Operands,
     return Error(getLoc(), "register list must start from 'ra' or 'x1'");
   getLexer().Lex();
 
-  bool SeenComma = parseOptionalToken(AsmToken::Comma);
-
-  // There are two choices here:
-  // - `s0` is not required (usual case), so only try to parse `s0` if there is
-  //   a 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 must be there if required)
-  if (SeenComma) {
+  if (parseOptionalToken(AsmToken::Comma)) {
     if (getLexer().isNot(AsmToken::Identifier))
       return Error(getLoc(), "invalid register");
     StringRef RegName = getLexer().getTok().getIdentifier();
@@ -2664,8 +2652,10 @@ ParseStatus RISCVAsmParser::parseRegListCommon(OperandVector &Operands,
 
   auto Encode = RISCVZC::encodeRlist(RegEnd, IsRVE);
   assert(Encode != RISCVZC::INVALID_RLIST);
-  if (MustIncludeS0)
-    assert(Encode != RISCVZC::RA);
+
+  if (MustIncludeS0 && Encode == RISCVZC::RA)
+    return Error(S, "register list must include 's0' or 'x8'");
+
   Operands.push_back(RISCVOperand::createRlist(Encode, S));
 
   return ParseStatus::Success;
diff --git a/llvm/test/MC/RISCV/rv32xqccmp-invalid.s b/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
index e43f86cbb84ee..5bfc2e3498bef 100644
--- a/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
+++ b/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
@@ -34,6 +34,6 @@ qc.cm.pushfp {ra, s0}, -12
 # CHECK-ERROR: :[[@LINE+1]]:24: 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: :[[@LINE+1]]:17: error: register list must include 's0' or 'x8'
+# CHECK-ERROR: :[[@LINE+1]]:14: error: register list must include 's0' or 'x8'
 qc.cm.pushfp {ra}, -16
 

[llvmbot]

@llvm/pr-subscribers-mc

Author: Craig Topper (topperc)

Changes

In my mind, this is more of a semantic check. I've also changed the diagnostic location to point at the register list start instead of the closing brace, or whatever character might be there instead of a brace if its malformed.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp (+5-15)
• (modified) llvm/test/MC/RISCV/rv32xqccmp-invalid.s (+1-1)

diff --git a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
index 7837504751694..ed890104d488d 100644
--- a/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
+++ b/llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
@@ -2588,20 +2588,8 @@ ParseStatus RISCVAsmParser::parseRegListCommon(OperandVector &Operands,
     return Error(getLoc(), "register list must start from 'ra' or 'x1'");
   getLexer().Lex();
 
-  bool SeenComma = parseOptionalToken(AsmToken::Comma);
-
-  // There are two choices here:
-  // - `s0` is not required (usual case), so only try to parse `s0` if there is
-  //   a 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 must be there if required)
-  if (SeenComma) {
+  if (parseOptionalToken(AsmToken::Comma)) {
     if (getLexer().isNot(AsmToken::Identifier))
       return Error(getLoc(), "invalid register");
     StringRef RegName = getLexer().getTok().getIdentifier();
@@ -2664,8 +2652,10 @@ ParseStatus RISCVAsmParser::parseRegListCommon(OperandVector &Operands,
 
   auto Encode = RISCVZC::encodeRlist(RegEnd, IsRVE);
   assert(Encode != RISCVZC::INVALID_RLIST);
-  if (MustIncludeS0)
-    assert(Encode != RISCVZC::RA);
+
+  if (MustIncludeS0 && Encode == RISCVZC::RA)
+    return Error(S, "register list must include 's0' or 'x8'");
+
   Operands.push_back(RISCVOperand::createRlist(Encode, S));
 
   return ParseStatus::Success;
diff --git a/llvm/test/MC/RISCV/rv32xqccmp-invalid.s b/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
index e43f86cbb84ee..5bfc2e3498bef 100644
--- a/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
+++ b/llvm/test/MC/RISCV/rv32xqccmp-invalid.s
@@ -34,6 +34,6 @@ qc.cm.pushfp {ra, s0}, -12
 # CHECK-ERROR: :[[@LINE+1]]:24: 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: :[[@LINE+1]]:17: error: register list must include 's0' or 'x8'
+# CHECK-ERROR: :[[@LINE+1]]:14: error: register list must include 's0' or 'x8'
 qc.cm.pushfp {ra}, -16
 

[lenary]

LGTM.