Page MenuHomePhabricator

[PPC][AIX] Add vector callee saved registers for AIX extended vector ABI
ClosedPublic

Authored by ZarkoCA on Oct 1 2020, 9:53 AM.

Details

Summary

This patch is the initial patch for support of the AIX extended vector ABI. The extended ABI treats vector registers V20-V31 as non-volatile and we add them as callee saved registers in this patch.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
ZarkoCA requested review of this revision.Oct 1 2020, 9:53 AM
sfertile added inline comments.
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
3

Minor nit: align this with the first argument in the preceeding line.

4

I'm not sure the formatting we have adopted for the tests, but I think subsequent commands were to be indented by 2? @Xiangling_L or @DiggerLin might know better, I think it was on one of their reviews where I saw it mentioned.

20

A couple minor suggestions

  1. Use only 2 or 3 callee saved vector registers as clobbers. It will show how we allocate stack space to store all vector registers from N to 31 where N is the lowest used csr vr, and make the test a bit more concise.
  2. Have V12 as a clobber to show that it is not callee saved.

More test coverage for this patch:

  1. We should test spill/restore code when we have gpr/fpr/vr callee-saves all in the same function.
  2. Extend at least one of the frame-pointer and base pointer tests to have a vector callee-saved clobbered.

See inline-asm-vsx-clobbers.ll. We should have a test which uses vsx registers to clobber fprs and vector registers and makes sure we allocate the right stack size, emit the right spills/restores. I think it should be a separate patch though as its testing the inline-asm implementation more, where the other test use inline-asm to test the frame-lowering. I can prepare this patch if you like.

sfertile added inline comments.Oct 1 2020, 11:05 AM
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
20

Ignore number 2. you already have that with v13, I should have paid more attention :(.

Xiangling_L added inline comments.Oct 1 2020, 2:09 PM
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
3

The ABI mentioned AIX5.3 is the first AIX release to enable vector programming, and there are arch like pwr4 is not compatible with altivec. Since this is our first altivec patch, it looks it's the right place to add report_fatal_error for arch level which doesn't support altivec.

4

The original formatting comment is here: https://reviews.llvm.org/D78929#inline-740137

223

Could you also add a testcase showing how csr FPR/GPR interact with nonvolatile VR?

ZarkoCA updated this revision to Diff 296527.Oct 6 2020, 12:54 PM

Added test case that includes FP and GP csrs along with the vector crs.
Reduced the number of clobbered VRs in the initial test case.

ZarkoCA marked 6 inline comments as done.Oct 6 2020, 1:05 PM
ZarkoCA added inline comments.
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
3

While I think that's a good suggestion, none of the other PPC targets do anything similar. If you choose an arch that doesn't support altivec while selecting a CPU that doesn't support it they quietly don't generate the altivec instructions.

Also, as things are, we do have a report fatal error when ever someone tries using vector types in the front end and in the back end.

4

Thanks for info, I updated the RUN steps.

Xiangling_L added inline comments.Oct 7 2020, 6:05 AM
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
3

I see. The only reason why I raise it up is because XL gives an error when using altivec with unsupported arch.

ZarkoCA marked an inline comment as done.Oct 7 2020, 6:26 AM
ZarkoCA added inline comments.
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
3

I see a warning and xlc and xlclang:
1506-1162 (W) The altivec option is not supported for the target architecture and is ignored.
Additionally with xlclang we get from the altivec.h header included in xlclang if an unsupported arch is specified.

But this has me thinking that it is a good idea to follow through with your suggestion of an error.

hubert.reinterpretcast added inline comments.
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
3

Since the extended ABI vector-enabled mode is not the safe default (certain call sequences involving functions compiled using the default ABI can bypass restoration of the non-volatile register values), we should report_fatal_error unless if the extended ABI is explicitly enabled.

Example:

[ uses non-volatile vector registers ]
vv   calls
[ not vector-extended ABI-aware ] -- calls setjmp
vv   calls
[ uses non-volatile vector registers ]
vv   calls
[ not vector-extended ABI-aware ] -- calls longjmp

This follows the precedent for the llc default for data sections: Even for llc, we do not enable the "unsafe" mode by default.

Xiangling_L added inline comments.Oct 7 2020, 8:45 AM
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
55

minor nit: trailing space on line 54, 55, 57, 58, 60

82

Can we line up all comments?

82

I am suggesting to use things like [[REG1:[0-9]+]] to match registers, use {{[0-9]+}} to match numerical values if we need to. The same comments apply to all testcases.

121

Thank you for adding this testcase. I think it would be better if we also test`r13`/x14, f14, v20, then we can observe the padding added in.

sfertile added inline comments.Oct 8 2020, 11:05 AM
llvm/lib/Target/PowerPC/PPCFrameLowering.cpp
220–221

Should we reformat this even though its ugly to avoid it cluttering other reviews that touch this file? Maybe if we delete the comment we will end up with a format that matches the 64-bit array more closely :shrug:

sfertile added inline comments.Oct 13 2020, 7:13 AM
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
41

STXVD2X killed $v20, $r{{[0-9]+}}, killed $r{{[0-9]+}} :: (store 16 into %fixed-stack.2)

Using $r{{[0-9]+}} for the second register in the r+r addressing is fine, but don't use a regex for r1/x1, its wothwhile to show that the addressing is based off he stack pointer.

Xiangling_L added inline comments.Oct 14 2020, 3:01 PM
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
5

The comments here let me think should we also implement an equivalent option for llc to control which ABI to be enabled in addition to the frontend or driver option?

ZarkoCA updated this revision to Diff 298409.Oct 15 2020, 10:13 AM
ZarkoCA retitled this revision from [PPC][AIX] Add vector callee saved registers for AIX extended vector ABI to [PPC][AIX] Add vector callee saved registers for AIX extended vector ABI and add clang and llvm option.
ZarkoCA edited the summary of this revision. (Show Details)

Added mvecnvol/mnovecnvol options in clang and vecnvol option in llc
Addressed other comments related to formatting and test case regex usage.
Updated test cases that fail when vecnvol is enabled.

Herald added a project: Restricted Project. · View Herald TranscriptOct 15 2020, 10:13 AM
ZarkoCA marked 5 inline comments as done.Oct 15 2020, 10:15 AM
ZarkoCA added inline comments.
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll
3

I added the options to toggle between the two Altivec ABIs.

5

Yes, good point, I added that as well.

82

I'd rather not use any variables unless we need to use them later.

121

Good suggestion, I added.

I am wondering can we split the option related changes to a separate patch for reviews? That would make current patch a bit easier to review and faster to be committed as two small pieces.

If it's possible, I am thinking we can try to split it up to the following two pieces:

  1. Add option in the frontend and backend to be able to turn on extended vector ABI
  2. Do the frame lowing in the backend
clang/docs/ClangCommandLineReference.rst
2868 ↗(On Diff #298409)
  1. I am not sure if it's a good idea to put the supporting status also in the option description here. It looks a bit strange to me.
  1. I would suggest something similar like this for the option description:
Only supported on AIX. Specifies whether to use both volatile and nonvolatile vector registers or volatile vector registers only. Defaults to `-mnovecnvol` when Altivec is enabled.
  1. We missed a - before mnovecnvol.
clang/include/clang/Basic/DiagnosticDriverKinds.td
531 ↗(On Diff #298409)

I would suggest:

The default Altivec ABI on AIX is not yet supported, use '-mvecnvol' for the extended Altivec ABI
clang/test/CodeGen/altivec.c
1 ↗(On Diff #298409)

Can we also test how the driver react to these two options? It would serve as the LIT coverage for the code change in clang/lib/Driver/ToolChains/Clang.cpp.

llvm/include/llvm/Target/TargetOptions.h
177 ↗(On Diff #298409)

Can we also use bitfield to indicate true and false here? The default value set to be false in ctor already, so we don't need assign 0 to it here.

unsigned AIXExtendedAltivecABI : 1;
ZarkoCA updated this revision to Diff 298991.Oct 19 2020, 3:32 AM
ZarkoCA marked 3 inline comments as done.
ZarkoCA retitled this revision from [PPC][AIX] Add vector callee saved registers for AIX extended vector ABI and add clang and llvm option to [PPC][AIX] Add vector callee saved registers for AIX extended vector ABI.
ZarkoCA edited the summary of this revision. (Show Details)

Separated the option portion of the previous diff (now found here: https://reviews.llvm.org/D89684).
Addressed other comments.

ZarkoCA updated this revision to Diff 298994.Oct 19 2020, 3:39 AM

Fixed typo in test case.

Harbormaster completed remote builds in B75514: Diff 298994.
sfertile added inline comments.Oct 27 2020, 9:51 AM
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
203

I suggest doing the error checking once before getting into the 32-bit/64-bit blocks.

ZarkoCA updated this revision to Diff 305150.Nov 13 2020, 8:00 AM

Rebased and addressed comments.

ZarkoCA marked an inline comment as done.Nov 13 2020, 8:02 AM
ZarkoCA added inline comments.
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
203

Thanks, good suggestion, I moved it up right at the beginning of the function.

sfertile added inline comments.Nov 18 2020, 12:13 PM
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
236

CSR_64_AllRegs_Altivec_RegMask should be CSR_PPC64_Altivec_RegMask. FWIW I don't think this is testable without D86476. If that's the case, then it should go in that patch, not this patch.

ZarkoCA added inline comments.Nov 18 2020, 12:36 PM
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
236

Are you suggesting that I also leave the error in if I were to move this change to D84676?

sfertile added inline comments.Nov 19 2020, 10:18 AM
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
236

Can you still run the tests that are part of this commit with that error in? My understanding was that it didn't interfere, but I didn't verify that. If we can still run the tests then yes leave the error in. If we can't then it probably gives us a clue about how to test the change in this patch without needing D84676, in which case we can keep the change and simply add the testing that exercises it.

ZarkoCA updated this revision to Diff 306515.Nov 19 2020, 1:08 PM
ZarkoCA marked an inline comment as done.

Rebase and remove regmask altivec change.

ZarkoCA marked 6 inline comments as done.Nov 19 2020, 1:10 PM
ZarkoCA added inline comments.
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp
236

No, you were right those changes can't be tested. Just needed some clarification, thanks.

ZarkoCA updated this revision to Diff 306545.Nov 19 2020, 3:17 PM
ZarkoCA marked an inline comment as done.

Fixed failing test cases.

sfertile accepted this revision.Nov 20 2020, 6:55 AM

LGTM.

This revision is now accepted and ready to land.Nov 20 2020, 6:55 AM
ZarkoCA updated this revision to Diff 307499.Nov 24 2020, 7:33 PM

Seems a few test cases require mattr=-altivec to pass with this patch.

This revision was landed with ongoing or failed builds.Nov 24 2020, 8:02 PM
This revision was automatically updated to reflect the committed changes.