Page MenuHomePhabricator

[PowerPC][AIX] Enable the default AltiVec ABI on AIX
ClosedPublic

Authored by ZarkoCA on Feb 9 2021, 9:09 AM.

Details

Summary

This patch adds support for the default AltiVec ABI for AIX.

Vector registers 20 through 31 are marked as reserved and cannot
be used in the default ABI. This patch adds handling for this case
and also remove the default AltiVec ABI errors.

Diff Detail

Event Timeline

ZarkoCA created this revision.Feb 9 2021, 9:09 AM
ZarkoCA requested review of this revision.Feb 9 2021, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2021, 9:09 AM
sfertile added inline comments.Feb 17 2021, 9:48 AM
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
202–203

Can we instead structure it this way?

if (TM.isPPC64()) {
  if (Subtarget.hasAltivec() &&
      (!Subtarget.isAIXABI() || TM.getAIXExtendedAltivecABI())) {
    return SaveR2 ? CSR_PPC64_R2_Altivec_SaveList
                  : CSR_PPC64_Altivec_SaveList;
  }
  return SaveR2 ? CSR_PPC64_R2_SaveList : CSR_PPC64_SaveList;
}
344

Use a range based for loop.

llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
215

The patch adds a lot of whitespace errors. I'm betting its from this mir output. Please highlight trailing whitespace and fix it up.

ZarkoCA updated this revision to Diff 324384.Feb 17 2021, 12:02 PM

Address comments:

  1. Remove trailing whitespaces
  2. Use a range based loop
  3. Clean up if use
ZarkoCA marked 3 inline comments as done.Feb 17 2021, 12:04 PM
ZarkoCA added inline comments.
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
202–203

I think your suggestion is simpler. Thanks, I was looking for ways to make it cleaner.

344

Thanks for catching this oversight by me.

llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
215

They should hopefully be gone now.

llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
59

Is this considered to "modify" the reserved registers that are marked as clobbers? I don't think D93659 should generate a traceback table that indicates that the reserved registers are saved in the save area if they are not.

llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
59

Looping back here:

  • We are okay to omit both the save and the restore here when a user insists on using the reserved registers (even though they are not supposed to). Note: XL oddly generates the save but not the restore.
  • We will try to generate a warning when the user specifies the reserved registers for inline assembly/explicit register variables.

Separately from this patch:

  • We need to fix the vector save/restore to save all of the registers in the range for the unwinder to restore.
  • We need to update the traceback table emission to signal cases where the field indicating the number of saved vector registers is reliable.
  • D93659 should be updated to not indicate any saved non-volatile vector registers when the default AIX vector ABI is used.
ZarkoCA updated this revision to Diff 325583.Feb 22 2021, 2:57 PM
ZarkoCA marked 3 inline comments as done.
  • Add warning when using reserved vector registers in the default AltiVec ABI
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
15338–15339

Is there a lot of precedent for "raw output" to stderr for warnings from the back-end? Can we diagnose this closer to where err_asm_unknown_register_name is handled in the front-end?

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
344

I'm not seeing a range-based for loop here.

I personally don't feel that the included test case changes are adequate for testing this patch. First of all, the test case probably does too much without equivalent checks - there is 32/64 bit, default/extended ABI and MIR/ASM testing all in a single file and the checks for default/extended at least do not appear to line up. There is no explicit checks that no spills/reloads exist for the reserved regs with default ABI.

Furthermore, I think you should have a test case where you clobber all FPR's and VSR's except the ones in the vr20 - vr31 range in functions that perform FP/vector computations with checks that show the affected registers are not allocated.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
15337

Seems like this should be producing a warning for a range of registers, but the condition covers an unterminated range. That seems odd and potentially wrong. Why is this not:

if ((R.first >= PPC::V20 && R.first <= PPC::V31) ||
    (R.first >= PPC::VF20 && R.first <= PPC::VF31))
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
348

Since this effectively shrinks the register class by 12 registers, it is worth double checking that marking these registers as reserved without providing an alternate allocation order does not adversely affect register pressure estimates.

ZarkoCA updated this revision to Diff 325958.Feb 23 2021, 7:04 PM

Address comments:

  • split extended and default Altivec ABI tests in different files
  • add a test case to check that reserved vector registers are not used even when all fprs and vector regs are clobbered
ZarkoCA marked 2 inline comments as done.Feb 23 2021, 7:30 PM

Furthermore, I think you should have a test case where you clobber all FPR's and VSR's except the ones in the vr20 - vr31 range in functions that perform FP/vector computations with checks that show the affected registers are not allocated.

I added all_fprs_and_vecregs and then checked to see that vr20-31 are not allocated. The check is only in MIR since there is no way to distinguish float, vector, and gprs in assembly on AIX by register number alone (we don't have ppc-full-reg-names on AIX yet).

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
15338–15339

I've seen some cases in other targets but, no, there is not a lot of precedent for this in PPC subtarget.

I investigated with making the suggested change where err_asm_unknown_register_name is handled in clang but it seems to me making the changes there is at least a patch of its own. I wasn't able to find an efficient way to do it for now using that method.

If you feel strongly about this, I could remove the warning here and then make a follow on patch to add the warning the front end. My own feeling is that the warning should come from the frond end and this is a comprise for now.

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
348

Is the way to test this to check the number of spills we get when using the different Altivec ABIs? In the test case all_fprs_and_vecregs I just added in llvm/test/CodeGen/PowerPC/aix-csr-vector.ll I didn't notice a difference in the number of registers spills. Would adding a check for that be sufficient?

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
15338–15339

I think having something here for now is better than having nothing. Please add a FIXME comment to implement the warning in the front-end to replace this one (and do the follow up patch after dealing with the functional issues on your plate for vector support right now).

Also, can you point me to the test for this?

ZarkoCA updated this revision to Diff 326075.Feb 24 2021, 6:46 AM

Add a test case for the warning message and add a FIXME to implement the warning in the front end.

ZarkoCA marked 2 inline comments as done.Feb 24 2021, 6:47 AM
ZarkoCA added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
15338–15339

Thanks Hubert, I added a FIXME and the test case for the warning message.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
15325
llvm/test/CodeGen/PowerPC/aix-csr-vector-extabi.ll
1 ↗(On Diff #326075)

If I'm getting the gist of this correctly, there's a plain rename that could be done instead. I suggest checking in the rename of the extabi test separately from this patch.

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
345–348

If the code worked before the switch to the range-based for loop, then I suspect it doesn't now.

llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
3

There's no need to have the DFL prefix is it is always used with the corresponding "base" prefix.

ZarkoCA updated this revision to Diff 326271.Feb 24 2021, 8:12 PM
ZarkoCA marked an inline comment as done.
  • Remove test added in NFC commit
  • fix loop code so now it works
ZarkoCA marked 2 inline comments as done.Feb 24 2021, 8:17 PM
ZarkoCA added inline comments.
llvm/test/CodeGen/PowerPC/aix-csr-vector-extabi.ll
1 ↗(On Diff #326075)

Thanks, this makes this patch simpler. Did this in commit 1c051b7b704257ed9a7

sfertile added inline comments.Mar 1 2021, 10:43 AM
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
4

Minor nit: when there is a single check prefix use --check-prefix=

27

Ideally we would either have check-nots for v20/v26/v31 before and after the INLINEASM instruction(or APP/NO_APP). Or maybe if you check for the basic-block label then check-next the following instructions to the blr might be an equivalent way to show the same understanding.

llvm/test/CodeGen/PowerPC/aix-inlineasm-reserved-reg-dflt-warn.ll
4

Minor nit: Just use 'CHECK:' so we can drop the --check-prefixes

ZarkoCA updated this revision to Diff 327296.Mar 1 2021, 2:43 PM

Added CHECK-NOT in test cases.
changed "check-prefixes" to "check-prefix"
Reworded one comment

ZarkoCA marked 2 inline comments as done.Mar 1 2021, 2:48 PM
ZarkoCA added inline comments.
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
27

I tried to address your comment as best as I could. Made it check for whether we load and store to those vector registers. In some cases it was not possible to reliably check for the vector registers since, for now, we can only check for the register number in assembly.

sfertile added inline comments.Mar 3 2021, 8:19 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
15325

I don't see this update.

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
348

This comment made me realize we need to update PPCRegisterInfo::getRegPressureLimit

ZarkoCA updated this revision to Diff 327828.Mar 3 2021, 9:39 AM

Update PPCRegisterInfo::getRegPressureLimit
Fixed comment typo missed previously

sfertile retitled this revision from [AIX] Enable the default AltiVec ABI on AIX to [PowerPC][AIX] Enable the default AltiVec ABI on AIX.Mar 4 2021, 12:26 PM
sfertile edited the summary of this revision. (Show Details)
sfertile added inline comments.Mar 4 2021, 2:58 PM
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
444

With this change we will need to split up these cases.

The pre VSX register sets are the easiest to explain:

F8RC --> This is the F0-F31, which represents the 32 fprs with type f64.
F4RC --> This is the same as F8RC but with type f32.

These register sets won't change in the default ABI and will still have 32 available registers.

VRRC --> This is V0-V31 and represents the vector registers. With the extended ABI we can access all of them, with the default ABI we have the limited set of the first 20.

Once we add VSX it gets complicated: the other register sets are how we represent the registers with VSX. VSX has 64 128-bit wide registers. The fprs are mapped into element 0 of the first 32 VSX registers (VSR0-VSR31), with the vrs are mapped into the the second set of 32 VSX registers (VSR32-VSR64).

VFRC --> This is the set of 32 64-bit floating point sub registers not overlapped by the fprs (vsr32-vsr64). Used in instructions like lxsd/stxsd where we encode a register number N between 0-31 in the instruction but the instruction targets the sub register 32 + N.

VSLRC --> The 32 128-bit registers that the fprs overlap (vs0-vsr31).

VSRC --> VSLRC U VR vsr0-vsr31 + vsr31-vsr63 = vsr0-vsr63, or all the vsx vector registers.
VSFRC --> VFRC U F8RC all the 64-bit sub registers of the vsx vector registers.
VSSRC --> Same as VSFRC but the with type f32.

The qvecnvol option doesn't mention vsx registers but I would assume any that overlap the VRs are affected the same way, and the scalar registers which overlap vsr0-vsr31 are unaffectedly by the option. I'm not sure if there is any restriction on the vector registers vsr0-vsr31 in the default ABI.

ZarkoCA updated this revision to Diff 328369.Mar 4 2021, 8:15 PM

Try to correct register pressure determination.

ZarkoCA added inline comments.Mar 4 2021, 8:25 PM
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
444

Thanks for this detailed explanation.
I updated the switch cases to reflect my understanding of this.

case PPC::VRRCRegClassID: {
    const PPCSubtarget &Subtarget = MF.getSubtarget<PPCSubtarget>();
    if (!TM.getAIXExtendedAltivecABI() && Subtarget.isAIXABI())
      return 20 - DefaultSafety;
  }
    LLVM_FALLTHROUGH;

The above seems straightforward to me but I can also change the fallthrough if it's not preferred.

I am less certain about this:

case PPC::VSRCRegClassID: {
  const PPCSubtarget &Subtarget = MF.getSubtarget<PPCSubtarget>();
  if (!TM.getAIXExtendedAltivecABI() && Subtarget.isAIXABI())
    return 64 - 12 - DefaultSafety;
}
  return 64 - DefaultSafety;

In the default ABI, VSX registers vsr54-63 should map to default abi registers v20-v31 and be reserved, and it's what I get from looking at PPCRegisterInfo.td.

343 def VRRC : RegisterClass<"PPC",
344                          [v16i8,v8i16,v4i32,v2i64,v1i128,v4f32,v2f64, f128],
345                          128,
346                          (add V2, V3, V4, V5, V0, V1, V6, V7, V8, V9, V10, V11,
347                              V12, V13, V14, V15, V16, V17, V18, V19, V31, V30,
348                              V29, V28, V27, V26, V25, V24, V23, V22, V21, V20)>;
...
355 def VSRC  : RegisterClass<"PPC", [v4i32,v4f32,v2f64,v2i64], 128,
356                           (add VSLRC, VRRC)>;
sfertile added inline comments.Mar 5 2021, 7:07 AM
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
443

VFRC is the sub registers of the vsx registers that 'overlap' the VRs, so when we only have 20 vr, then we only have 20 subregistes as well.

447

I think it best to avoid the fallthrough.

464

All three of these reg classes will have the same count, since if we reduce the vector registers by 12, then we also lose the sub registers of those vector registers.

ZarkoCA updated this revision to Diff 328540.Mar 5 2021, 8:44 AM

Correct switch case

ZarkoCA marked 4 inline comments as done.Mar 5 2021, 8:45 AM
etiotto added a subscriber: etiotto.Mar 5 2021, 8:47 AM
sfertile accepted this revision.Mar 5 2021, 9:09 AM

One minor comment but otherwise LGTM.

llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
460

Really minor nit: Either just use available register count like the other cases (ie 52 - DefaultSafety) or name the constant 12 something descriptive like 'DefaultAIXABIReservedVRCount` and use it in both places we have reduced register counts.

This revision is now accepted and ready to land.Mar 5 2021, 9:09 AM
ZarkoCA updated this revision to Diff 328553.Mar 5 2021, 9:23 AM

Use reduce register count.

This revision was landed with ongoing or failed builds.Mar 5 2021, 9:46 AM
This revision was automatically updated to reflect the committed changes.