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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:366
+                       "'Zcmt' (table jump instuctions for code-size reduction)", 
+                       [FeatureExtZca]>; // TODO: add Zicsr as another dependence
+def HasStdExtZcmt : Predicate<"Subtarget->hasStdExtZcmt() && !Subtarget->hasStdExtC()">,
----------------
This is an odd implication, Zcmt works just fine without Zca?


================
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.
Shouldn't the conflict be enforced at a higher level? Otherwise you're silently making +c,+zcmt resolve to +c here.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:19
+//===----------------------------------------------------------------------===//
+def uimm8 : Operand<XLenVT>,
+              ImmLeaf<XLenVT, [{return isUInt<8>(Imm);}]> {
----------------
Blank line


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:20
+def uimm8 : Operand<XLenVT>,
+              ImmLeaf<XLenVT, [{return isUInt<8>(Imm);}]> {
+  let ParserMatchClass = UImmAsmOperand<8>;
----------------
This should line up?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:28
+      return false;
+    return MCOp.isBareSymbolRef();
+  }];
----------------
I don't think we want this? `cm.jt sym` isn't a meaningful thing.


================
Comment at: llvm/lib/Target/RISCV/RISCVSystemOperands.td:383
+//===-----------------------------------------------
+// Table jump base CSR
+//===-----------------------------------------------
----------------
Capitalise (and maybe Jump Vector Table, assuming that's what jvt stands for? these headings come from the priv spec but that's woefully outdated for all the new asciidoc extensions...)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133863