Page MenuHomePhabricator

[AIX] Return the correct set of callee saved regs
ClosedPublic

Authored by daltenty on Mar 30 2020, 3:45 PM.

Details

Summary

r13 isn't reserved on 32-bit AIX, which is reflected in our calling
convention but not callee saved regs.

Diff Detail

Event Timeline

daltenty created this revision.Mar 30 2020, 3:45 PM
daltenty updated this revision to Diff 253727.Mar 30 2020, 3:46 PM
  • Add newline to test

Change looks good, however there is a couple things complicating this.

  1. The SplitCSR code is dead with Darwins removal and we didn't realize it.
  2. CSR_AIX64 and CSR_SVR464 are the same and should be one list, not 2 distinct lists.

Usually I would suggest we land 2 NFC patches to clean this up first, however I don't want to delay this bugfix for te removal of dead code.

If no one else objects, I would suggest:

  1. Condensing the SVR464 and AIX64 lists to a single list and removing the Darwin lists in an NFC patch.
  2. Updating this patch to: keep the fatal error you added for AnyReg calling convention, then handling isOSAIX() immediately after, with a fatal error if the calling convention is ColdCC.

That avoids changing the SplitCSR code which makes it look like its still alive for SVR4 ABIs. We can then remove SplitCSR in a follow up NFC patch.

daltenty planned changes to this revision.Apr 1 2020, 9:18 AM

Posted the NFC patch suggested as D77235

If no one else objects, I would suggest:

  1. Condensing the SVR464 and AIX64 lists to a single list and removing the Darwin lists in an NFC patch.
  2. Updating this patch to: keep the fatal error you added for AnyReg calling convention, then handling isOSAIX() immediately after, with a fatal error if the calling convention is ColdCC.

    That avoids changing the SplitCSR code which makes it look like its still alive for SVR4 ABIs. We can then remove SplitCSR in a follow up NFC patch.
daltenty updated this revision to Diff 255722.Apr 7 2020, 10:38 AM
  • Remove 64-bit cases since we've commoned up the AIX64 and SVR4 CSRs

M

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
154–156

Minor nit: can we add an error for SplitCC on AIX as well?

182

This error should be for both 64-bit and 32-bit AIX targets (We will need to land lit tests + rename the SVR64_ColdCC before removing the error for 64-bit).

daltenty updated this revision to Diff 258058.Apr 16 2020, 8:08 AM
daltenty marked an inline comment as done.
  • Add SplitCSR and ColdCall errors
daltenty marked an inline comment as done.Apr 16 2020, 8:08 AM
sfertile accepted this revision.Apr 17 2020, 9:57 AM

LGTM.

This revision is now accepted and ready to land.Apr 17 2020, 9:57 AM
lei added a subscriber: lei.Apr 17 2020, 3:09 PM
lei added inline comments.
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
179

nit: unrelated format change.

daltenty marked an inline comment as done.Apr 20 2020, 8:18 AM
daltenty updated this revision to Diff 258754.Apr 20 2020, 8:21 AM

Remove unrelated reformatting

This revision was automatically updated to reflect the committed changes.

This breaks tests: http://45.33.8.238/linux/15714/step_12.txt

Please take a look and revert if it takes a while to investigate.

This breaks tests: http://45.33.8.238/linux/15714/step_12.txt

Please take a look and revert if it takes a while to investigate.

Reverted in 6c881bf1fec2288907cd87a7895c863243bba7c5.

It broke hardcoded stack offsets in test aix-cc-byval-mem.ll, the test is fixed as of 8541a3cc9dca52933f71baa5f344a0b815638b6a. Relanding.