This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Adjust CSR order to avoid breaking ABI regarding traceback
ClosedPublic

Authored by lkail on Apr 9 2021, 2:01 AM.

Details

Summary

Allocate non-volatile registers in order to be compatible with ABI, regarding gpr_save.

Quoted from https://www.ibm.com/docs/en/ssw_aix_72/assembler/assembler_pdf.pdf page55,

The preferred method of using GPRs is to use the volatile registers first. Next, use the nonvolatile registers
in descending order, starting with GPR31.

This patch is based on @jsji 's initial draft.

Tested on test-suite and SPEC, found no degradation.

Diff Detail

Event Timeline

lkail created this revision.Apr 9 2021, 2:01 AM
lkail requested review of this revision.Apr 9 2021, 2:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2021, 2:01 AM
lkail edited the summary of this revision. (Show Details)Apr 9 2021, 2:01 AM
lkail edited the summary of this revision. (Show Details)
lkail edited the summary of this revision. (Show Details)
lkail edited the summary of this revision. (Show Details)Apr 9 2021, 2:04 AM
lkail updated this revision to Diff 336361.Apr 9 2021, 2:23 AM
jsji added inline comments.Apr 26 2021, 7:32 PM
llvm/lib/Target/PowerPC/PPCRegisterInfo.td
307

Add comments about the AIX sequence.

313

Please make this a Subtarget function, something like PPCSubtarget::getGPRAllocationOrder, so that we don't need to duplicate code here.

llvm/test/CodeGen/PowerPC/inc-of-add.ll
165

Why removing these lines?

Please add some info about how this affects functions that need the frame pointer. Do we get more/less saves/restores? Does something go wrong?

llvm/lib/Target/PowerPC/PPCRegisterInfo.td
313

The name should indicate that it is the index into the list of allocation orders. So maybe PPCSubtarget::getGPRAllocationOrderIdx()

lkail added inline comments.Jun 29 2021, 7:32 PM
llvm/test/CodeGen/PowerPC/inc-of-add.ll
165

I think it should be due utils/update_llc_test_checks.py is unable to handle different output with a same check prefix. I'll re-generate check for AIX.

lkail updated this revision to Diff 355449.Jun 29 2021, 10:23 PM

Address comments.

lkail marked 3 inline comments as done.EditedJun 29 2021, 10:24 PM

Please add some info about how this affects functions that need the frame pointer. Do we get more/less saves/restores? Does something go wrong?

Looks nothing changed if a function needs frame pointer. No change found in test/CodeGen/PowerPC/aix-framepointer-save-restore.ll.

lkail marked an inline comment as done.Jun 29 2021, 10:25 PM
lkail updated this revision to Diff 355450.Jun 29 2021, 10:44 PM
lkail updated this revision to Diff 355453.Jun 29 2021, 11:36 PM
shchenz added inline comments.
llvm/lib/Target/PowerPC/PPCSubtarget.h
421

Can we use 0 here? 0 is not a valid allocation order index in the td file?

llvm/test/CodeGen/PowerPC/aix-csr-alloc.ll
9

In the allocation order definition, $x2 is allocatable, but here we don't have it. Maybe we can also use $x2 on AIX like what we did for ELF? Yeah, this should be not related to this patch, we can do it in another follow-up.

lkail added inline comments.Jun 30 2021, 12:36 AM
llvm/lib/Target/PowerPC/PPCSubtarget.h
421

0 corresponds to the default allocation order which is defined as the last argument of RegisterClass.
This can be checked in PPCGenRegisterInfo.inc

static inline unsigned GPRCAltOrderSelect(const MachineFunction &MF) {
    return MF.getSubtarget<PPCSubtarget>().getGPRAllocationOrderIdx();
  }

static ArrayRef<MCPhysReg> GPRCGetRawAllocationOrder(const MachineFunction &MF) {
  static const MCPhysReg AltOrder1[] = { PPC::R3, PPC::R4, PPC::R5, PPC::R6, PPC::R7, PPC::R8, PPC::R9, PPC::R10, PPC::R11, PPC::R12, PPC::R30, PPC::R29, PPC::R28, PPC::R27, PPC::R26, PPC::R25, PPC::R24, PPC::R23, PPC::R22, PPC::R21, PPC::R20, PPC::R19, PPC::R18, PPC::R17, PPC::R16, PPC::R15, PPC::R14, PPC::R13, PPC::R31, PPC::R0, PPC::R1, PPC::FP, PPC::BP, PPC::R2 };
  static const MCPhysReg AltOrder2[] = { PPC::R2, PPC::R3, PPC::R4, PPC::R5, PPC::R6, PPC::R7, PPC::R8, PPC::R9, PPC::R10, PPC::R11, PPC::R12, PPC::R31, PPC::R30, PPC::R29, PPC::R28, PPC::R27, PPC::R26, PPC::R25, PPC::R24, PPC::R23, PPC::R22, PPC::R21, PPC::R20, PPC::R19, PPC::R18, PPC::R17, PPC::R16, PPC::R15, PPC::R14, PPC::R13, PPC::R0, PPC::R1, PPC::FP, PPC::BP };
  const MCRegisterClass &MCR = PPCMCRegisterClasses[PPC::GPRCRegClassID];
  const ArrayRef<MCPhysReg> Order[] = {
    makeArrayRef(MCR.begin(), MCR.getNumRegs()),
    makeArrayRef(AltOrder1),
    makeArrayRef(AltOrder2)
  };
  const unsigned Select = GPRCAltOrderSelect(MF);
  assert(Select < 3);
  return Order[Select];
}
lkail added inline comments.Jun 30 2021, 1:51 AM
llvm/test/CodeGen/PowerPC/aix-csr-alloc.ll
9
// Always reserve r2 on AIX for now.
// TODO: Make r2 allocatable on AIX/XCOFF for some leaf functions.
if (Subtarget.isAIXABI())
  markSuperRegs(Reserved, PPC::R2);  // System-reserved register

Looks so, good point.

qiucf added a subscriber: qiucf.Jun 30 2021, 2:37 AM
qiucf added inline comments.
llvm/test/CodeGen/PowerPC/inc-of-add.ll
165

Will D97106 help?

lkail added inline comments.Jun 30 2021, 2:45 AM
llvm/test/CodeGen/PowerPC/inc-of-add.ll
165

Looks it can't be applied cleanly and need rebase. Get through the patch, I think it would help. It's better than keeping silence when updating tests.

ZarkoCA accepted this revision.Jun 30 2021, 5:58 AM

LGTM, thanks for rebasing and updating the patch. Just some comment suggestions.
I guess you may want to wait for the other reviewers to check if their comments were addressed to their liking.

llvm/lib/Target/PowerPC/PPCRegisterInfo.td
304–312
320

We can also remove references to Darwin here and below since there isn't support for that PPC Subtarget in llc anymore. But this is just a suggestion as this isn't the main point of the patch.

llvm/test/CodeGen/PowerPC/aix-tracetable-csr.ll
17–20

Thank you for rebasing the patch because now this test is included and it shows exactly what this patch fixes with the traceback table.

This revision is now accepted and ready to land.Jun 30 2021, 5:58 AM
jsji accepted this revision as: jsji.Jun 30 2021, 7:12 AM

LGTM. Thanks.

Have we checked the allocation of non-volatile FPRs and VRs? Is it always in descending order, i.e. starting with FPR31 or VR31?

lkail updated this revision to Diff 355748.EditedJun 30 2021, 7:16 PM
  • Adjust comments
  • Per @xingxue 's comment, show allocation orders of float and vector registers.
xingxue accepted this revision.Jul 2 2021, 1:43 PM

LGTM, thanks!