[PATCH] D132819: [RISCV] Add MC support of RISCV zcmp Extension

[Craig Topper via Phabricator via cfe-commits]

craig.topper added a comment.

Need to update RISCVUsage.rst



================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:969
+    case KindTy::Spimm:
+      OS << "{Spimm: ";
+      RISCVZC::printSpimm(Spimm.Val, OS);
----------------
Why curly braces when everything else uses `<` and `>`?


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1057
+
+  static std::unique_ptr<RISCVOperand> createSpimm(unsigned spimm, SMLoc S,
+                                                   bool IsRV64) {
----------------
Capitalize `spimm`


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2339
+    if (RegStart != RISCV::X1)
+      return MatchOperand_NoMatch;
+    getLexer().Lex();
----------------
Should we be using ParseFail with more specific error messages instead of NoMatch once we start calling Lex() on tokens? If you use NoMatch, I think we need to unwind the tokens that have been lexed.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:513
+
+inline unsigned encodeRlist(MCRegister EndReg, bool isCInst,
+                            bool IsRV32E = false) {
----------------
isCInst and IsRV32E are unused.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:515
+                            bool IsRV32E = false) {
+  auto RlistEncode = [=] {
+    switch (EndReg) {
----------------
Get rid of the lambda. Create a variable and assign to it in the switch.


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:550
+
+inline static unsigned getStackAdjBase(unsigned rlistVal, bool isRV64,
+                                       bool isEABI) {
----------------
Capitalize variable names


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:554
+    assert(0 && "{ra, s0-s10} is not supported, s11 must be included.");
+  if (isEABI) {
+    return 16;
----------------
Drop curly braces around single line if


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:602
+
+inline static bool getSpimm(unsigned rlistVal, unsigned &spimmVal,
+                            int64_t stackAdjustment, bool isRV64, bool isEABI) {
----------------
Capitalize variable names


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:494
+  MCOperand MO = MI.getOperand(OpNo);
+  assert(MO.isImm() && "Rlist operand must be immidiate");
+  auto Imm = MO.getImm();
----------------
immidiate -> immediate


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:356
+    : SubtargetFeature<"experimental-zcmp", "HasStdExtZcmp", "true",
+                       "'Zcmp' (sequenced instuctions for code-size reduction.)", 
+                       [FeatureStdExtZca]>;
----------------
Drop the `.` after reduction


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:360
+                               AssemblerPredicate<(all_of FeatureStdExtZcmp, (not FeatureStdExtC)),
+                               "'Zcmp' (sequenced instuctions for code-size reduction.)">;
+
----------------
Drop the . after reduction


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:132
+let hasSideEffects = 0, mayLoad = 1, mayStore = 0 in
+class ZcLoad_ri<bits<3> funct3, bits<2> opcode, string opcodestr,
+                 RegisterClass cls, DAGOperand opnd>
----------------
This isn't used


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:192
 
+// ZCMP
+let Predicates = [HasStdExtZcmp], Defs = [X10, X11],
----------------
ZCMP -> Zcmp


================
Comment at: llvm/test/CodeGen/RISCV/attributes.ll:73
 
+
 ; RUN: llc -mtriple=riscv64 %s -o - | FileCheck %s
----------------
Drop this change


================
Comment at: llvm/test/MC/RISCV/rv32zcmp-invalid.s:5
+# CHECK-ERROR: error: invalid operand for instruction
+cm.mvsa01 a1, a2
+
----------------
Do we need to check that we don't except `mvsa01 s0, s0`? The spec says the two s registers must be different.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132819