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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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
|
More test coverage for this patch:
- We should test spill/restore code when we have gpr/fpr/vr callee-saves all in the same function.
- 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.
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll | ||
---|---|---|
20 | Ignore number 2. you already have that with v13, I should have paid more attention :(. |
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? |
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.
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. |
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. |
llvm/test/CodeGen/PowerPC/aix-csr-vector.ll | ||
---|---|---|
3 | I see a warning and xlc and xlclang: But this has me thinking that it is a good idea to follow through with your suggestion of an error. |
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. |
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. |
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: |
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. |
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? |
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.
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:
- Add option in the frontend and backend to be able to turn on extended vector ABI
- Do the frame lowing in the backend
clang/docs/ClangCommandLineReference.rst | ||
---|---|---|
2868 ↗ | (On Diff #298409) |
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.
|
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; |
Separated the option portion of the previous diff (now found here: https://reviews.llvm.org/D89684).
Addressed other comments.
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp | ||
---|---|---|
203 | I suggest doing the error checking once before getting into the 32-bit/64-bit blocks. |
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp | ||
---|---|---|
203 | Thanks, good suggestion, I moved it up right at the beginning of the function. |
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. |
llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp | ||
---|---|---|
236 | No, you were right those changes can't be tested. Just needed some clarification, thanks. |
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: