[PATCH] D95588: [RISCV] Implement the MC layer support of P extension

[Luís Marques via Phabricator via llvm-commits]

luismarques added a comment.

This patch is nearly there! Just address the remaining review comments and it LGTM.

BTW, please mark all addressed inline comments as done. I think a few were missed, and it's helpful for a large patch like this.



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:953-956
+static MCRegister convertGPRToGPRPair(MCRegister Reg) {
+  assert(isGPRPair(Reg) && "Invalid register");
+  return (Reg - RISCV::X0) / 2 + RISCV::X0_ZERO;
+}
----------------
Nitpicking: perhaps this could use better terminology. You're asserting that `Reg` is a GPRPair, and then you convert `Reg` to a GPRPair, but the return value would fail an assert of `isGPRPair`...


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:69
 
+def sub_lo : SubRegIndex<32>;
+def sub_hi : SubRegIndex<32, 32>;
----------------
jrtc27 wrote:
> This assumes RV32, and is not clear it applies to register pairs
What's the best way to address this?


================
Comment at: llvm/test/MC/RISCV/rv32zpsfoperand-invalid.s:6-9
+# Paired register operand
+
+# CHECK-ERROR: error: invalid operand for instruction
+smal a0, a1, a2
----------------
It would be nice to have a more specific error message, indicating the need for an even register operand. But not a deal-breaker, IMO.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95588/new/

https://reviews.llvm.org/D95588