[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/RISCVPushPopOptimizer.cpp:32
+  const TargetRegisterInfo *TRI;
+  const RISCVSubtarget *Subtarget;
+
----------------
`Subtarget` doesn't need to be a member. It's only used in runOnMachineFunction


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:41
+
+  DenseMap<MachineInstr *, int> retValMap;
+
----------------
Capitalize variable names


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:70
+    DebugLoc DL = NextI->getDebugLoc();
+    auto retValInfo = retValMap.find(&(*MBBI));
+    if (retValInfo == retValMap.end())
----------------
Why does this need to be map? This function and adjustRetVal are called on the same basic block. There can't be values for any other basic block in the map. You could probably just pass the bool result from adjustRetVal to this function?


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:75
+          .add(MBBI->getOperand(1));
+    else if (retValInfo->second == 0)
+      BuildMI(*NextI->getParent(), NextI, DL, TII->get(RISCV::CM_POPRETZ))
----------------
If the value exists, it must be zero right?


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:108
+      if (DestReg == RISCV::X10 && Source == RISCV::X0 &&
+          MI.getOperand(2).getImm() == 0) {
+        retValMap[&(*MBBI)] = 0;
----------------
Isn't `MI.getOperand(2).getImm() == 0` guaranteed by `TII->isCopyInstrImpl(MI)`?


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:150
+    if (MBBI != MBB.end() && MBB.isReturnBlock()) {
+      Modified |= adjustRetVal(MBBI);
+      Modified |= usePopRet(MBBI);
----------------
I think you need to know the terminator is PseudoRET before you can call adjustRetVal. PseudoTAIL, PseudoTAILIndirect, SRET, and MRET also have `isReturn` but are not eligible for usePopRet.


================
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)) {
----------------
craig.topper wrote:
> How does it break the ABI?
This question was not answered.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150416