[PATCH] D40001: [RISCV] MC layer support for the load/store instructions of standard compress instruction set

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Hi Shiva, I think we're almost there. Primarily naming and style nits below. The most important issue to resolve is the implied SP operand I think.



================
Comment at: lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:97
+// in bit field).
+static void addImplySP(MCInst &Inst, int64_t Address, const void *Decoder) {
+  if (Inst.getOpcode() == RISCV::CLWSP || Inst.getOpcode() == RISCV::CSWSP ||
----------------
Is there a reason we can't use `let Uses = [X2]` and/or `let Defs = [X2]` for instructions that implicitly read/write the stack pointer register?


================
Comment at: lib/Target/RISCV/RISCVInstrFormatsC.td:37
+
+class CI<bits<3> funct3, bits<2> opcode, dag outs, dag ins,
+         string opcodestr, string argstr>
----------------
I started off by using very short class names in RISCVInforFormats.td, but found it's ultimately easy to confuse a class name with an instruction. RVInstCI or RVInst16CI and so on would probably be preferable.


================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:67
+  let Inst{6-4} = imm{4-2};
+  let Inst{3-2} = imm{7-6};
+}
----------------
shiva0217 wrote:
> asb wrote:
> > Maybe I'm missing something, but can't the CL class take care of placing these immediate bits where appropriate (same for the instructions below)
> The reason we put immediate encode in CLW is that CLD use the same encoding format(CL) except the immediate encoding is different.
> To reuse the format class, we extract the immediate encoding in each instruction.
I see. In that case, it would make sense to add a comment to the CL and CS classes that explains subclasses must set the immediate bits. An alternative could be to define different classes for these variants as is done with RVInstIShift and RVInstIShiftW.


================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:16
+
+// A 7-bit unsigned immediate where the least two bits are zero.
+def uimm7_lsb00 : Operand<XLenVT>,
----------------
"where the least significant two bits are zero." (same fix for other operand types defined below).


================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:53
+let hasSideEffects = 0, mayLoad = 1, mayStore = 0 in
+class Stack_Load<bits<3> funct3, string OpcodeStr,
+                 RegisterClass cls, DAGOperand opnd> :
----------------
`StackLoad_ri` or `CStackLoad would match the naming convention used in RISCVInstrInfo.td and others. Same for the other classes. See also my query regarding the sp operand and whether we can use Defs and Uses instead.


================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:80
+
+def CLWSP  : Stack_Load<0b010, "c.lwsp", GPR, uimm8_lsb00>,
+             Requires<[HasStdExtC]> {
----------------
It would match the other RISCVInstrInfo*.td files use `let Predicates = [HasStdExtC]` in a way similar to a C++ namespace.


================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:80
+
+def CLWSP  : Stack_Load<0b010, "c.lwsp", GPR, uimm8_lsb00>,
+             Requires<[HasStdExtC]> {
----------------
asb wrote:
> It would match the other RISCVInstrInfo*.td files use `let Predicates = [HasStdExtC]` in a way similar to a C++ namespace.
Just one space is needed after the instruction name and before :


================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:86
+
+def CSWSP  : Stack_Store<0b110, "c.swsp", GPR, uimm8_lsb00>,
+             Requires<[HasStdExtC]> {
----------------
I try to keep the MC layer instruction definitions as close to the ISA manual as possible, ordering based on the the quickref table on page 104 of the spec. It might be nice to order based on the tables in section 12.7 "RVC Instruction Set Listings" (page 82 and 83). There's probably less benefit in doing so than for the RV32IMAFD extensions though, so it's up to you. Generally, it's nice to be able to see at a glance that every instruction in that listing is represented.


================
Comment at: test/MC/RISCV/rv32c-invalid.s:10
+## uimm8_lsb00
+c.lwsp  ra, 300(sp) # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 4 bytes in the range [0, 252]
+c.swsp  ra, -20(sp) # CHECK: :[[@LINE]]:13: error: immediate must be a multiple of 4 bytes in the range [0, 252]
----------------
It would be good to check values that are just outside the valid range.


================
Comment at: test/MC/RISCV/rv32c-valid.s:3
+# RUN:     | FileCheck -check-prefixes=CHECK,CHECK-INST %s
+# RUN: llvm-mc -triple=riscv64 -mattr=+c,+64bit -show-encoding < %s \
+# RUN:     | FileCheck -check-prefixes=CHECK,CHECK-INST %s
----------------
There's no need to explicitly state +64bit in this or the other tests.


================
Comment at: test/MC/RISCV/rv32c-valid.s:13
+# CHECK: encoding: [0xb2,0x40]
+c.lwsp  ra, 12(sp)
+# CHECK-INST: c.swsp  ra, 12(sp)
----------------
It would be good to check the maximum and minimum immediate values (same with other *-valid.s tests).


https://reviews.llvm.org/D40001