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

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Support/RISCVISAInfo.cpp:116
     {"zca", RISCVExtensionVersion{0, 70}},
+    {"zcb", RISCVExtensionVersion{0, 70}},
+    {"zcmp", RISCVExtensionVersion{0, 70}},
----------------
Why is `zcb` mixed into this patch?


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:311
+  struct RlistOp {
+    bool isCInst;
+    unsigned Val;
----------------
Capitalize


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp:189
+void RISCVZC::printRlist(unsigned SlistEncode, raw_ostream &OS) {
+  OS << "{";
+  switch (SlistEncode) {
----------------
Can we generate this more programmatically than having 12 unique strings?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp:193
+  int64_t Imm = MI->getOperand(OpNo).getImm();
+  unsigned opcode = MI->getOpcode();
+  bool isRV64 = STI.getFeatureBits()[RISCV::Feature64Bit];
----------------
Capitalize variables


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp:199
+  if (rlistVal == 16)
+    assert(0 && "Incorrect rlist.");
+  auto base = RISCVZC::getStackAdjBase(rlistVal, isRV64, isEABI);
----------------
assert(rlistVal != 16 && "Incorrect rlist.");


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:414
+  if (Imm < 4)
+    assert(0 && "EABI is currently not implemented");
+  else
----------------
assert(Imm >= 4 && "EABI is currently not implemented");


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1077
+Optional<DestSourcePair>
+RISCVInstrInfo::isLoadImmImpl(const MachineInstr &MI) const {
+  if (MI.isMoveImmediate())
----------------
This is not MC layer and there's no caller in this patch.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:9
+///
+/// This file describes the RISC-V instructions from the 'Zc' Code-size 
+/// reduction extension, version 0.70.4.
----------------
This isn't completely accurace since Zca is in RISCVInstrInfoC.td


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:136
+      let Inst{1-0} = 0b10;
+      let Inst{15-13} = 0b101;
+}
----------------
too much indentation


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:189
+// Registers saveable by PUSH instruction in Zc extension
+def PGPR : RegisterClass<"RISCV", [XLenVT], 32, (add
+    (sequence "X%u", 8, 9),
----------------
Where is this used?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132819