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

[Xinlong Wu via Phabricator via llvm-commits]

VincentWu marked 6 inline comments as done.
VincentWu added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp:150
+    if (MBBI != MBB.end() && MBB.isReturnBlock()) {
+      Modified |= adjustRetVal(MBBI);
+      Modified |= usePopRet(MBBI);
----------------
craig.topper wrote:
> 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.
Yeah you are right, that's why I have whether `NextI->getOpcode() == RISCV::PseudoRET` at func `adjustRetVal`.

I will move it to here )


================
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?
it was mentioned here https://github.com/plctlab/llvm-project/issues/58 ,
but I'm not sure whether it is the best solution


================
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)) {
----------------
jrtc27 wrote:
> craig.topper wrote:
> > jrtc27 wrote:
> > > craig.topper wrote:
> > > > VincentWu wrote:
> > > > > craig.topper wrote:
> > > > > > How does it break the ABI?
> > > > > it was mentioned here https://github.com/plctlab/llvm-project/issues/58 ,
> > > > > but I'm not sure whether it is the best solution
> > > > This question was not answered.
> > > https://github.com/riscv/riscv-code-size-reduction/issues/194 and https://github.com/riscv-non-isa/riscv-elf-psabi-doc/issues/18#issuecomment-1304496707 onwards
> > As far as I can tell, this patch just fuses the cm.pop with ret. Wouldn't the ABI break occur at the creation of cm.pop not at the formation of cm.popret?
> Yes, using cm.push/pop is the issue, not fusing ones that already exist. Haven't looked at the content of the patch other than the few lines of context here.
yes, So you can find the same code in the patch D134599#change-ZdluzfeA7hef

This code here is just a quick exit if it doesn't match the condition


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150416