[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.

Many many thanks for the patch update Shiva, this is looking pretty good. I have a concern in the comment about the immediate encoding for C.LWSP.

Shouldn't rv32c-invalid.s also check for cases like c.lwsp with rd as x0?

Thanks for clarifying the reasoning behind implySP - I can see now why you implemented things that way. Let me think a little more about it.



================
Comment at: lib/Target/RISCV/RISCVInstrFormatsC.td:52
+
+// Immediate value encoding would different for each instruction
+// which should set by each subclass.
----------------
Perhaps phrase as something like "The immediate value encoding differs for each instruction, so each subclass is responsible for setting the appropriate bits in the Inst field." In fact, it would be even better to document the bits that must be set, e.g. Inst{12-10}.


================
Comment at: lib/Target/RISCV/RISCVInstrInfoC.td:113
+  let Inst{3-2} = imm{7-6};
+}
+
----------------
What about Inst{12}? I think we need more tests in rv32c-valid.s that verify the immediate encoding is correct.


Repository:
  rL LLVM

https://reviews.llvm.org/D40001