Page MenuHomePhabricator

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

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



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

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


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


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


Can this be a lamda or function?


Please add comments about the mapping from GPR to VSR?


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


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


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


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


several GPRs? 1 or 2?

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

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

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


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


Please avoid asserts without text explaining the assert.


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


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.Mon, Apr 12, 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.Mon, Apr 12, 10:00 AM
nemanjai accepted this revision.Tue, Apr 13, 3:13 AM


This revision was automatically updated to reflect the committed changes.