Page MenuHomePhabricator

PowerPC: Fix register spilling for SPE registers

Authored by jhibbits on Jan 14 2019, 8:38 PM.



Missed in the original commit, use the correct callee-saved register
list for spilling, instead of the standard SVR432 list. This avoids
needlessly spilling the SPE non-volatile registers when they're not used.

Diff Detail


Event Timeline

jhibbits created this revision.Jan 14 2019, 8:38 PM
nemanjai requested changes to this revision.Feb 20 2019, 4:33 AM

I think this is very close, but I really think the reloading code needs to be fixed.

1161 ↗(On Diff #181713)

This does not look right. We are in RC == nullptr section here. Why are we checking hasSubClassEq(nullptr)?

163 ↗(On Diff #181713)

These ternary operator expressions were arguably too complex already. I don't think there's any doubt that after this they're becoming nearly unparsable by a human. Can you please refactor these to use nested if's?

This revision now requires changes to proceed.Feb 20 2019, 4:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2019, 4:33 AM
jhibbits updated this revision to Diff 188754.Feb 28 2019, 10:30 AM
jhibbits marked 2 inline comments as done.

Address feedback. Update patch to provide full context.

nemanjai requested changes to this revision.Mar 31 2019, 5:21 AM

Can you add a test case? At least for the spills that you're adding.

This revision now requires changes to proceed.Mar 31 2019, 5:21 AM

Can you add a test case? At least for the spills that you're adding.

Is there a good example for a test case testing register spills with no determined register class? That's the only location where I added new spill handling. The real meat of this should already be handled by the existing regression tests.

jhibbits updated this revision to Diff 206986.Jun 27 2019, 6:52 PM

Reduce the scope of the patch.

jhibbits edited the summary of this revision. (Show Details)Jun 28 2019, 7:16 AM

I removed the extra spill changes. Those can be done later if there actually is a problem. This didn't change the tests at all. I'm not quite sure how to write a test to demonstrate that "some but not all" registers are spilled in a given case, given that the register allocator can change at any time, so can't hard-code the registers that would get spilled, and I don't think I can even hard-code the number of registers that could get spilled.

LGTM other than the code here looks really messy - but it looks messy regardless of this patch.

I think after this lands, we should refactor this stuff to something like this:

const MCPhysReg *CSRLists[] = {

// Compute the index into the array based on Subtarget features, calling convention, ABI, the need to save R2, and object mode (32/64 bit).
return CSRLists[ComputedIdx];

and similarly for the regmasks.

170 ↗(On Diff #206986)

No need for else when the previous if just does a return. The same comment applies to all the else statements here.

// Cold calling convention CSR's.
if (MF->getFunction().getCallingConv() == CallingConv::Cold) {
  // 64-bit targets.
  if (TM.isPPC64()) {
    if (Subtarget.hasAltivec())
      return SaveR2 ? ...
    return SaveR2 ? ...
  // 32-bit targets.
  if (Subtarget.hasAltivec())
    return CSR_SVR32_ColdCC_Altivec_SaveList;
  if (Subtarget.hasSPE())
    return CSR_SVR32_ColdCC_SPE_SaveList;
  return CSR_SVR32_ColdCC_SaveList

// Standard calling convention CSR's.
nemanjai accepted this revision.Jul 16 2019, 1:54 PM

Oops. Forgot to accept.

This revision is now accepted and ready to land.Jul 16 2019, 1:54 PM
This revision was automatically updated to reflect the committed changes.