This is an archive of the discontinued LLVM Phabricator instance.

[PowerPC][AIX] Spill CSRs to the ABI specified stack offsets.
ClosedPublic

Authored by sfertile on May 1 2020, 10:50 AM.

Details

Diff Detail

Event Timeline

sfertile created this revision.May 1 2020, 10:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2020, 10:50 AM
sfertile updated this revision to Diff 261865.May 4 2020, 10:20 AM

Updated aix-calleesaved.ll test.

ZarkoCA added inline comments.May 8 2020, 11:18 AM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
230

Is the extra CSR only applicable to 32BIT AIX? Eg. is X13 not a CSR in 64BIT AIX?

llvm/test/CodeGen/PowerPC/aix-csr.ll
176–182

What are the other tests in this file showing that this one doesn't? It seems to me this test makes at least the gprs_only test redundant.

cebowleratibm added inline comments.May 8 2020, 11:41 AM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
231

nit: remove blank line.

1836

Since we're enabling this path for AIX is there a LIT test that we should add an AIX variant for to validate this shrink-wrap functionality?

2000

const or fold it into the use. This change is NFC and could be committed independently.

llvm/test/CodeGen/PowerPC/aix-cc-byval-mem.ll
206

Why do we expect this change to bump the stack adj? I thought the cc tests don't touch non-volatile regs.

ZarkoCA added inline comments.May 8 2020, 12:44 PM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
230

Thanks, I was using Table 1 here as a reference: https://www.ibm.com/support/knowledgecenter/ssw_aix_72/assembler/idalangref_prologs_epilogs.html but I suppose the information there is for 32BIT only. Even though it's not specified.

sfertile updated this revision to Diff 263193.May 11 2020, 9:29 AM
sfertile marked 9 inline comments as done.
  • Removed extra whitespace.
  • Removed redundant testing (of fprs saves)
sfertile updated this revision to Diff 263223.May 11 2020, 11:22 AM

rebased to pick up NFC commit.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
1836

Shrink-wrapping is only enabled for 64-bit ELF for now. I intend to enable it eventually, but not in this patch,

bool PPCFrameLowering::enableShrinkWrapping(const MachineFunction &MF) const {
  if (MF.getInfo<PPCFunctionInfo>()->shrinkWrapDisabled())
    return false;
  return (MF.getSubtarget<PPCSubtarget>().isSVR4ABI() &&
          MF.getSubtarget<PPCSubtarget>().isPPC64());
}

I can add atest where we shrink wrap for ELF 64-bit and then check we don't shrink wrap for AIX if you want, but I didn't think it was necessary.

2000

Good point, will update and commit separately.

llvm/test/CodeGen/PowerPC/aix-cc-byval-mem.ll
206

Some of them do: For example in this test, we load the address of the global before the call to memcpy. This means the compiler has to use a non-volatile (r30) to keep the value live across the call. By saving r30 to its ABI specified offset, we increase the stack frame size by 4, and to keep the stack aligned the size has to grow by 16. It's similar case for the other tests that change.

llvm/test/CodeGen/PowerPC/aix-csr.ll
176–182

The gprs_only test shows that we calculate the offsets to gprs when he fpr save area is non-existent. The fpr only test is redundant though, I'll remove it.

LGMT, I will give some time for other reviewers to have a look before I approve.

llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
231

nit: From what I've seen the TODO format is to do use a colon TODO:

ZarkoCA accepted this revision.May 20 2020, 12:03 PM
This revision is now accepted and ready to land.May 20 2020, 12:03 PM
This revision was automatically updated to reflect the committed changes.