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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:326
+    unsigned Val;
+    bool IsRV64;
+  };
----------------
Is this IsRV64 field used?


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:331
+    unsigned Val;
+    bool IsRV64;
+  };
----------------
Is this IsRV64 field used?


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1028
+  // be called after checking isFRMArg.
+  RISCVFPRndMode::RoundingMode getRoundingMode() const {
+    // isFRMArg has validated the operand, meaning this cast is safe.
----------------
`getRoundingMode` shouldn't be in this patch


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2120
+    StringRef EndName = getLexer().getTok().getIdentifier();
+    // FixMe: the register mapping and checks of EABI is wrong
+    if (matchRegisterNameHelper(/*IsEABI*/ false, RegEnd, EndName))
----------------
FixMe -> FIXME


================
Comment at: llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp:451
+                                  const void *Decoder) {
+  // Sign-extend the number in the bottom N bits of Imm
+  if (Imm <= 3)
----------------
This comment looks copy/pasted from somewhere else?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.cpp:224
+    else if (SlistEncode > 5 && SlistEncode <= 14)
+      OS << "-s" + std::to_string(SlistEncode - 5);
+  }
----------------
Does this work? 

```
OS << "-s" << (SlistEncode - 5);
```


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVBaseInfo.h:492
+  RA_S0_S11,
+  RA_S0_S10, // This is for error checking.
+};
----------------
Should use this define instead writing 16 in several places? Or maybe rename this so it's obviously invalid?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp:200
+  if (Spimm < Base || Spimm > Base + 48)
+    llvm_unreachable("Incorrect spimm");
+  if (Opcode == RISCV::CM_PUSH) {
----------------
Combine the `if` into an `assert`?


================
Comment at: llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:97
+private:
+  FeatureBitset computeAvailableFeatures(const FeatureBitset &FB) const;
+  void
----------------
What are these functions?


================
Comment at: llvm/lib/Target/RISCV/RISCVFeatures.td:335
+def HasStdExtZcmp : Predicate<"Subtarget->hasStdExtZcmp() && !Subtarget->hasStdExtC()">,
+                           AssemblerPredicate<(all_of FeatureExtZcmp, (not FeatureStdExtC)),
+                           "'Zcmp' (sequenced instuctions for code-size reduction.)">;
----------------
Line `AssemblerPredicate` up with the `"Subtarget` on the previous line.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:70
+      return false;
+    return  isShiftedUInt<5, 4>(Imm);
+  }];
----------------
Extra space before `isShiftedUInt`


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:85
 
+let hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+class RVZcArith_rr<bits<6> funct6,bits<2> funct2, bits<2> opcode, string opcodestr>
----------------
These lines were deleted when the Zcb patch was merged.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoZc.td:260
                   (C_ZEXT_H GPRC:$rs1, GPRC:$rs1)>;
-} // Predicates = [HasStdExtZcb, HasStdExtZbb]
+} // Predicates = [, HasStdExtZbb]
 
----------------
What happened here?


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterInfo.td:177
 
+// Saved Registers from s0 to s7, for C.MVA01S07 instruction in Zc extension
+def SR07 : RegisterClass<"RISCV", [XLenVT], 32, (add
----------------
Zc -> Zcmp?


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedRocket.td:21
   let UnsupportedFeatures = [HasStdExtZbkb, HasStdExtZbkc, HasStdExtZbkx,
-                             HasStdExtZknd, HasStdExtZkne, HasStdExtZknh,
-                             HasStdExtZksed, HasStdExtZksh, HasStdExtZkr,
-                             HasVInstructions, HasVInstructionsI64];
+                             HasStdExtZcb, HasStdExtZknd, HasStdExtZkne,
+                             HasStdExtZknh, HasStdExtZksed, HasStdExtZksh,
----------------
While this is probably correct, since it says Zcb and not Zcmp, it doesn't belong in this patch.


================
Comment at: llvm/lib/Target/RISCV/RISCVSchedSiFive7.td:18
   let CompleteModel = 0;
   let UnsupportedFeatures = [HasStdExtZbkb, HasStdExtZbkc, HasStdExtZbkx,
+                             HasStdExtZcb, HasStdExtZknd, HasStdExtZkne,
----------------
Same


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:83
   initializeRISCVInsertVSETVLIPass(*PR);
-  initializeRISCVDAGToDAGISelPass(*PR);
 }
----------------
????


================
Comment at: llvm/test/MC/RISCV/attribute-arch.s:170
 
-.attribute arch, "rv32izihintntl0p2"
-# CHECK: attribute      5, "rv32i2p0_zihintntl0p2"
----------------
??????


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132819