[PATCH] D130141: [RISCV] Add part support of RISCV Zc Extension (Zca)

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/Object/ELFObjectFile.cpp:317
 
+    HasZca = Arch.contains("zca");
     while (!Arch.empty()) {
----------------
This doesn't seem right to me. Why is `zca` treated this way but no other `z*` extensions are?


================
Comment at: llvm/lib/Target/RISCV/RISCVFrameLowering.h:48
 
+  bool isCSIpushable(const std::vector<CalleeSavedInfo> &CSI) const;
+
----------------
`Pushable` with a capital `P`

EDIT: but this function isn't used?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:937
+  const Align FunctionAlignment(
+      (Subtarget.hasStdExtC() || Subtarget.hasStdExtZca()) ? 2 : 4);
   setMinFunctionAlignment(FunctionAlignment);
----------------
Should we have `Subtarget.hasStdExtCOrStdExtZca`?

In certain contexts, would even a `Subtarget.hasCInstructions` make sense, like we have `Subtarget.hasVInstructions`?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:61
 MCInst RISCVInstrInfo::getNop() const {
-  if (STI.getFeatureBits()[RISCV::FeatureStdExtC])
+  if (STI.getFeatureBits()[RISCV::FeatureStdExtC] ||
+      STI.getFeatureBits()[RISCV::FeatureExtZca])
----------------
Do we need to resort to checking feature bits in `RISCVInstrInfo`? Can't we use `STI.hasStdExtC` and friends?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1246
   unsigned FrameOverhead = 4;
-  if (RepeatedSequenceLocs[0].getMF()->getSubtarget()
-          .getFeatureBits()[RISCV::FeatureStdExtC])
+  if (RepeatedSequenceLocs[0]
+          .getMF()
----------------
I'd rather the fetching of the subtarget was taken out into a variable rather than repeating it twice.

Also can we cast this to a `RISCVSubtarget` and use `hasStdExtC` and friends instead of prodding feature bits?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130141