[PATCH] D131141: [RISCV] Add MC support of RISCV Zcb Extension

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:169
+
+let isCompressOnly = true in {
+
----------------
This feels wrong to me, but decompression is a bit dodgy if you can have all of Zcb without some of the extensions that its instructions decompress to?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:177
+                  (C_MUL GPRC:$rs1, GPRC:$rs2)>;
+} //Predicates = [HasStdExtZcb, HasStdExtMOrZmmul]
+
----------------
Space after // (repeated multiple times)


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:195
+
+// zext.b
+let Predicates = [HasStdExtZcb] in{
----------------
These comments are pointless (repeated multiple times)


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:215
+def : CompressPat<(LBU  GPRC:$rd, GPRC:$rs1, uimm2_zc:$imm),
+                  (C_LBU GPRC:$rd, GPRC:$rs1, uimm2_zc:$imm)>;
+
----------------
These don't line up, either pad them properly or don't bother at all


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:239
+let Predicates = [HasStdExtZcb] in {
+  def : InstAlias<"c.lbu $rd, (${rs1})",  (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
+  def : InstAlias<"c.lhu $rd, (${rs1})",  (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
----------------
These aren't indented anywhere in llvm/lib/Target/RISCV


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:240
+  def : InstAlias<"c.lbu $rd, (${rs1})",  (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
+  def : InstAlias<"c.lhu $rd, (${rs1})",  (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
+  def : InstAlias<"c.lh $rd, (${rs1})",   (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
----------------
Why do you have at least two spaces after the comma here for every line? If you want to make them line up then there should be at least one where there's only one space.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131141