This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC] Exploiting to use mtvsrdd instruction when save called-saved GPR register to VSR registers
ClosedPublic

Authored by qiucf on May 28 2019, 8:21 PM.

Details

Summary

Make an exploiting to use mtvsrdd instruction when save called-saved GPR registers to VSR registers.
Because mtvsrdd can save two GPRs together at one time and make the space/time more efficient.

Diff Detail

Event Timeline

wuzish created this revision.May 28 2019, 8:21 PM
jsji added inline comments.Jun 10 2019, 2:49 PM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
2134

The comment are comparing to old implementation? This is unnecessary?

2142

No need to check hasP9Vector() here, we already early exit above when !Subtarget.hasP9Vector() in line 2107.

2152

Similar to above, we only enable spill to VSR for P9, so no need to check here.

2185

Can this be a lamda or function?

2190

Please add comments about the mapping from GPR to VSR?

2256

Do we want to save the two GPRs in the order of endian order? or maybe fixed litten endian?

2266

Never use assert(0...), that's what llvm_unreachable is for.

2430

Maybe the order here should be align with the restore order?

2440

Never use assert(0), that's what llvm_unreachable is for.

llvm/lib/Target/PowerPC/PPCFrameLowering.h
32

several GPRs? 1 or 2?

llvm/test/CodeGen/MIR/PowerPC/prolog_vec_spills.mir
16 ↗(On Diff #201814)

Please add comments about checking points.

23 ↗(On Diff #201814)

It looks weird to have x14 before x15, although they should be the same.

jsji requested changes to this revision.Jun 25 2019, 9:17 AM
This revision now requires changes to proceed.Jun 25 2019, 9:17 AM
qiucf commandeered this revision.Oct 11 2019, 7:25 PM
qiucf added a reviewer: wuzish.
jsji added a reviewer: Restricted Project.Jan 30 2020, 7:18 AM
jsji added a project: Restricted Project.
qiucf updated this revision to Diff 260210.Apr 26 2020, 9:16 PM
qiucf marked 7 inline comments as done.

Rebase and address some basic comments

qiucf updated this revision to Diff 325704.Feb 23 2021, 1:03 AM
qiucf marked 3 inline comments as done.

Address comments

qiucf added inline comments.Feb 24 2021, 1:14 AM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
2256

Here it always uses big-endian element order to save and restore, so it doesn't matter?

nemanjai added inline comments.Mar 1 2021, 10:57 AM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
2259

This name is strange. We are not spilling VSR's here. Perhaps LastVSRUsedForSpill?

2310

I think an assert to ensure we are not trying to put more than two GPRs in a VSR should be added here.

2375

Please avoid asserts without text explaining the assert.

llvm/lib/Target/PowerPC/PPCFrameLowering.h
33

I am curious why this maps from unsigned. Why not from Register?
Also, I think it makes sense to map to std::pair since there will be (at most) two GPR's.

Finally, I think the name is confusing. I understand that it is mapping a VSR to GPRs, but since this optimization is moving stuff between registers, it can easily be misunderstood to mean that we are moving a VSR to GPRs. I think a better name would be something like VSRContainsGPRs.

qiucf updated this revision to Diff 328042.Mar 4 2021, 12:05 AM
qiucf marked 4 inline comments as done.

Address comments from Nemanja

llvm/lib/Target/PowerPC/PPCFrameLowering.h
33

Ah, I just used unsigned to align with class CalleeSavedInfo definition:

class CalleeSavedInfo {
  Register Reg;
  union {
    int FrameIdx;
    unsigned DstReg;
  };
  //...
};
jsji accepted this revision as: jsji.Apr 12 2021, 10:00 AM

LGTM, please wait a few days in case @nemanjai has more comments. Thanks.

This revision is now accepted and ready to land.Apr 12 2021, 10:00 AM
nemanjai accepted this revision.Apr 13 2021, 3:13 AM

LGTM.

This revision was automatically updated to reflect the committed changes.