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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:366
+                       "'Zcb' (Shortened format for basic bit manipulation instructions)", 
+                       [FeatureExtZca]>;
+def HasStdExtZcb : Predicate<"Subtarget->hasStdExtZcb()">,
----------------
Should this implication also be in RISCVISAInfo.cpp? Right now I think specifying Zcb in the -march will put zca in the ELF attributes, but won't define __riscv_zca.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:120
+def C_LBU : ZcLoad_ri<0b100, 0b00, "c.lbu", GPRC, uimm2_zc>,
+          Sched<[]> {
+bits<2> imm;
----------------
Use the Sched information for LBU.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:121
+          Sched<[]> {
+bits<2> imm;
+
----------------
Please indent the body of this.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:128
+def C_LHU : ZcLoad_ri<0b100, 0b00, "c.lhu", GPRC, uimm2_lsb0>,
+          Sched<[]> {
+bits<2> imm;
----------------
Use the Sched information for LHU.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:131
+
+let Inst{12-10} = 0b001;
+let Inst{6} = 0b0;
----------------
Indent


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:138
+          Sched<[]> {
+bits<2> imm;
+
----------------
Indent


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:161
+}
+}
----------------
Need InstAliases for load/store with no immediate specified. Similar to this alias from RISCVInstrInfoC.td

```
def : InstAlias<"c.lw $rd, (${rs1})", (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
```


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedSiFive7.td:19
   let UnsupportedFeatures = [HasStdExtZbkb, HasStdExtZbkc, HasStdExtZbkx,
-                             HasStdExtZknd, HasStdExtZkne, HasStdExtZknh,
-                             HasStdExtZksed, HasStdExtZksh, HasStdExtZkr,
-                             HasVInstructions];
+                             HasStdExtZcb, HasStdExtZknd, HasStdExtZkne,
+                             HasStdExtZknh, HasStdExtZksed, HasStdExtZksh,
----------------
If we put the write Sched information to match the uncompressed forms, is this still needed?


================
Comment at: llvm/test/MC/RISCV/rv32zcb-valid.s:13
+# CHECK-ASM: encoding: [0x61,0x9c]
+# CHECK-NO-EXT: error: instruction requires the following: 'Zcb' (Shortened format for basic bit manipulation instructions)
+c.zext.b s0
----------------
Please check to end of line using `{{$}}`


================
Comment at: llvm/test/MC/RISCV/rv64zcb-invalid.s:1
+# RUN: not llvm-mc -triple=riscv64 -mattr=experimental-zcb -riscv-no-aliases -show-encoding < %s 2>&1 \
+# RUN:     | FileCheck -check-prefixes=CHECK-ERROR %s
----------------
Use a single test file for riscv32 and riscv64


================
Comment at: llvm/test/MC/RISCV/rv64zcb-valid.s:1
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+m,+zbb,+zba,+experimental-zcb -riscv-no-aliases -show-encoding \
+# RUN:     | FileCheck -check-prefixes=CHECK-ASM,CHECK-ASM-AND-OBJ %s
----------------
Use a single test file for riscv32 and riscv64. Except for riscv64 only instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131141