This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add a pass to combine `cm.pop` and `ret` insts
ClosedPublic

Authored by VincentWu on May 11 2023, 6:59 PM.

Details

Summary

RISCVPushPopOptimizer.cpp combine cm.pop and ret to generates cm.popretz or cm.popret .

Diff Detail

Event Timeline

VincentWu created this revision.May 11 2023, 6:59 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 11 2023, 6:59 PM
VincentWu requested review of this revision.May 11 2023, 6:59 PM
KYG added a subscriber: KYG.May 17 2023, 12:47 AM
KYG added inline comments.May 17 2023, 12:58 AM
llvm/lib/Target/RISCV/RISCVInstrInfo.h
134 ↗(On Diff #521526)

Could we just use isCopyInstrImpl instead?
Since it's doing almost the same thing, and there's no prototype in TargetInstrInfo.h.

craig.topper added inline comments.May 17 2023, 12:22 PM
llvm/lib/Target/RISCV/RISCVInstrInfo.h
134 ↗(On Diff #521526)

Agreed. Can we use isCopyInstrImpl and check for a copy from X0?

llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp
42

RetVal map should be capitalized.

And this should probably be a DenseMap.

135

use Fn.getSubtarget<RISCVSubtarget>

136

Drop curly braces

140

The line break in this comment should be closer to column 80.

141

How does it break the ABI?

142

Drop curly braces

158

If it's not a return block, and adjustRetVal deletes the LI instruction, where does the return value go?

llvm/lib/Target/RISCV/RISCVTargetMachine.cpp
351

Should this be skipped at -O0?

evandro removed a subscriber: evandro.May 17 2023, 3:50 PM
VincentWu updated this revision to Diff 526423.May 29 2023, 3:46 AM
VincentWu marked 9 inline comments as done.

address comments

craig.topper added inline comments.May 30 2023, 3:53 PM
llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp
32

Subtarget doesn't need to be a member. It's only used in runOnMachineFunction

41

Capitalize variable names

70

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?

75

If the value exists, it must be zero right?

108

Isn't MI.getOperand(2).getImm() == 0 guaranteed by TII->isCopyInstrImpl(MI)?

141

This question was not answered.

150

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.

craig.topper added inline comments.May 30 2023, 4:08 PM
llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp
141

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?

jrtc27 added inline comments.May 30 2023, 4:09 PM
llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp
141

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.

VincentWu marked 6 inline comments as done.Jun 5 2023, 8:37 PM
VincentWu added inline comments.
llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp
141

it was mentioned here https://github.com/plctlab/llvm-project/issues/58 ,
but I'm not sure whether it is the best solution

141

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

150

Yeah you are right, that's why I have whether NextI->getOpcode() == RISCV::PseudoRET at func adjustRetVal.

I will move it to here )

VincentWu marked an inline comment as done.Jun 5 2023, 8:38 PM
VincentWu added inline comments.
llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp
141

it was mentioned here https://github.com/plctlab/llvm-project/issues/58 ,
but I'm not sure whether it is the best solution

Sorry for late reply, I forgot to submit it

VincentWu marked 5 inline comments as done.Jun 16 2023, 9:31 PM

I see comments are marked done, but the patch wasn't updated?

VincentWu updated this revision to Diff 533925.Jun 23 2023, 4:59 AM

address comments

This revision is now accepted and ready to land.Jun 24 2023, 2:07 PM
craig.topper added inline comments.Jun 24 2023, 2:13 PM
llvm/lib/Target/RISCV/RISCVPushPopOptimizer.cpp
68

Can we do?

unsigned Opc = IsReturnZero ? RISCV::CM_POPRETZ : RISCV::CM_POPRET;
BuildMI(*NextI->getParent(), NextI, DL, TII->get(Opc))
        .add(MBBI->getOperand(0))
        .add(MBBI->getOperand(1));

address comments

VincentWu marked an inline comment as done.Jun 25 2023, 12:52 AM
VincentWu updated this revision to Diff 537985.Jul 6 2023, 10:08 PM

rebase & update testcase
ready to land

This revision was landed with ongoing or failed builds.Jul 6 2023, 11:04 PM
This revision was automatically updated to reflect the committed changes.