[PATCH] D150416: [RISCV] Add a pass to combine `cm.pop` and `ret` insts

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.h:134
 
+  std::optional<DestSourcePair> isLoadImmImpl(const MachineInstr &MI) const;
+
----------------
KYG wrote:
> Could we just use `isCopyInstrImpl` instead?
> Since it's doing almost the same thing, and there's no prototype in TargetInstrInfo.h.
> 
Agreed. Can we use isCopyInstrImpl and check for a copy from X0?


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:41
+
+  std::map<MachineInstr *, int> retValMap;
+
----------------
RetVal map should be capitalized.

And this should probably be a DenseMap.


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:134
+  // If Zcmp extension is not supported, abort.
+  Subtarget = &static_cast<const RISCVSubtarget &>(Fn.getSubtarget());
+  if (!Subtarget->hasStdExtZcmp()) {
----------------
use `Fn.getSubtarget<RISCVSubtarget>`


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:135
+  Subtarget = &static_cast<const RISCVSubtarget &>(Fn.getSubtarget());
+  if (!Subtarget->hasStdExtZcmp()) {
+    return false;
----------------
Drop curly braces


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:139
+
+  // If frame pointer elimination has been disabled,
+  // abort to avoid breaking the ABI.
----------------
The line break in this comment should be closer to column 80.


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:140
+  // If frame pointer elimination has been disabled,
+  // abort to avoid breaking the ABI.
+  if (Fn.getTarget().Options.DisableFramePointerElim(Fn)) {
----------------
How does it break the ABI?


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:141
+  // abort to avoid breaking the ABI.
+  if (Fn.getTarget().Options.DisableFramePointerElim(Fn)) {
+    return false;
----------------
Drop curly braces


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:157
+      Modified |= adjustRetVal(MBBI);
+      if (MBB.isReturnBlock())
+        Modified |= usePopRet(MBBI);
----------------
If it's not a return block, and adjustRetVal deletes the LI instruction, where does the return value go?


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetMachine.cpp:351
 void RISCVPassConfig::addPreEmitPass2() {
+  // Schedule PushPop Optimization before expansion of Pseudo instruction,
+  // ensuring return instruction is detected correctly.
----------------
Should this be skipped at -O0?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150416