[PATCH] D133863: [RISCV] Add MC support of RISCV zcmt Extension

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:367
+                       [FeatureExtZca]>; // TODO: add Zicsr as another dependence
+def HasStdExtZcmt : Predicate<"Subtarget->hasStdExtZcmt() && !Subtarget->hasStdExtC()">,
+                           AssemblerPredicate<(all_of FeatureExtZcmt, (not FeatureStdExtC)),
----------------
craig.topper wrote:
> reames wrote:
> > Wait, Zcmt can't be enabled if C is?  That seems odd...
> I think Zcmt reuses encodings from the FP part of C. That's why C was split into Zca, Zcf, and Zcd.
This restriction needs to be enforced in RISCVISAInfo.cpp so that it generates a proper error message.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:1
+//===-- RISCVInstrInfoZc.td - RISC-V 'Zc' instructions -----*- tablegen -*-===//
+//
----------------
reames wrote:
> We have Zca in RISCVInstrInfoC.td.  Not sure it makes sense to have Zcmt in one place, and Zca in another. 
> 
> Also, did Zca change between 0.7 and 0.7.5?  If so, any plans to update that extension?
I think all Zc* should be in RISCVInstrInfoC.td. Zca/Zcf/Zcd already have to be in that file. Might as well put the rest there.

We should mention the Zc* extensions in the header of that file.


================
Comment at: llvm/lib/Target/RISCV/RISCVSystemOperands.td:386
+
+def : SysReg<"jvt", 0x017>;
----------------
This needs an assembler test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133863