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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

Please add to the review description a link to the appropriate specification.  Please update docs/RISCVUsage.rst to add Zcmt, and link to the specification.  It's impossible to review e.g. encoding without knowing what you're implementing.



================
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)),
----------------
Wait, Zcmt can't be enabled if C is?  That seems odd...


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:1
+//===-- RISCVInstrInfoZc.td - RISC-V 'Zc' instructions -----*- tablegen -*-===//
+//
----------------
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?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:10
+/// This file describes the RISC-V instructions from the 'Zc' Code-size 
+/// reduction extension, version 0.70.5.
+/// This version is still experimental as the 'Zc' extension hasn't been
----------------
It's not just one extension.  It is several sub-extensions.

Please reword as "from the Zc* family of code size reduction extensions, version ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133863