[PATCH] D130141: [RISCV] Add MC support of RISCV Zca Extension

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:332
+    : SubtargetFeature<"experimental-zca", "HasStdExtZca", "true",
+                       "'Zca' (part of the C extension, excluding all 16-bit floating point loads and stores)">;
+def HasStdExtZca : Predicate<"Subtarget->hasStdExtZca()">,
----------------
`16-bit` in this is potentially confusing as it could also be read as the size of the data being loaded/stored. "compressed" might be more clear?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoC.td:408
 
-let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0, Predicates = [HasStdExtCOrZca]in
 def C_LI : RVInst16CI<0b010, 0b01, (outs GPRNoX0:$rd), (ins simm6:$imm),
----------------
Is adding Predicates here fixing a bug? Or was it getting the one on line 284 making this change redundant?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoC.td:495
 
 def C_LWSP : CStackLoad<0b010, "c.lwsp", GPRNoX0, uimm8_lsb00>,
              Sched<[WriteLDW, ReadMemBase]> {
----------------
Doesn't `C_LWSP` need to be updated? It's sharing with `C_FLDSP`


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoC.td:555
 
 def C_SWSP : CStackStore<0b110, "c.swsp", GPR, uimm8_lsb00>,
              Sched<[WriteSTW, ReadStoreData, ReadMemBase]> {
----------------
`C_SWSP` needs to be updated


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoC.td:702
 
 def : InstAlias<"c.lw $rd, (${rs1})", (C_LW GPRC:$rd, GPRC:$rs1, 0)>;
 
----------------
There's no predicate in effect for c.lw. This is a bug already in tree. I'll fix it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130141