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

Wed Nov 22 23:33:08 PST 2017

shiva0217 added a comment.

Hi Alex, `if (RISCV::GPRRegClass.hasSubClassEq(RC))` in storeRegToStackSlot and loadRegtoStackSlot looks like a good a solution to me. About imply SP I have explained in the inline comments, let me know if you have other thought about it. Thanks for your comments, I really appreciate that.

================
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 ||
----------------
asb wrote:
> 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?
The reason is that InstPrinter has to print SP operand "c.lwsp ra, 12(sp)".
We could print sp as string and remove it in input operand list rather than add imply SP,
but the parser will parse sp as input operand and we'll need to add other logic to deal with it.
If we add imply SP operand while disassembling, then printer and parser could work fine without extra code.

================
Comment at: lib/Target/RISCV/RISCVInstrFormatsC.td:37
+
+class CI<bits<3> funct3, bits<2> opcode, dag outs, dag ins,
+         string opcodestr, string argstr>
----------------
asb wrote:
> 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.
Rename the instruction formats for compress instructions with "RVInst16" prefix.

================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:67
+  let Inst{6-4} = imm{4-2};
+  let Inst{3-2} = imm{7-6};
+}
----------------
asb wrote:
> 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.
Adding comments to CL and CS classes that subclasses must set the immediate bits.

================
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>,
----------------
asb wrote:
> "where the least significant two bits are zero." (same fix for other operand types defined below).
Fixup comments.

================
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> :
----------------
asb wrote:
> `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.
Rename the class to `CStackLoad` to match the naming rule in RISCVInstrInfo.td.

================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:80
+
+def CLWSP  : Stack_Load<0b010, "c.lwsp", GPR, uimm8_lsb00>,
+             Requires<[HasStdExtC]> {
----------------
asb wrote:
> 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 :
Using `let Predicates = [HasStdExtC]` instead of using require.

================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:86
+
+def CSWSP  : Stack_Store<0b110, "c.swsp", GPR, uimm8_lsb00>,
+             Requires<[HasStdExtC]> {
----------------
asb wrote:
> 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.
OK, I'll list the instructions base on the table in section 12.7 "RVC Instruction Set Listings" (page 82 and 83). Let me know if I still misorder something.

================
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]
----------------
asb wrote:
> It would be good to check values that are just outside the valid range.
Testing the invalid value 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
----------------
asb wrote:
> There's no need to explicitly state +64bit in this or the other tests.
Remove redundant +64bit argument in the testcase.

================
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)
----------------
asb wrote:
> It would be good to check the maximum and minimum immediate values (same with other *-valid.s tests).
check the maximum and minimum immediate values in -valids tests.

Repository:
  rL LLVM

https://reviews.llvm.org/D40001