[PATCH] D88676: [PPC][AIX] Add vector callee saved registers for AIX extended vector ABI and add clang and llvm option
Xiangling_L added a comment. 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 Comment at: clang/docs/ClangCommandLineReference.rst:2868 +Specify usage of volatile and nonvolatile vector registers, the extended vector ABI on AIX (AIX only). The default AIX vector ABI is not yet supported. + 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. 2. 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. ``` 3. We missed a `-` before `mnovecnvol`. Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:531 +def err_aix_default_altivec_abi : Error< + "The default Altivec ABI on AIX is not yet supported, use the extended ABI option '-mvecnvol'">; + I would suggest: ``` The default Altivec ABI on AIX is not yet supported, use '-mvecnvol' for the extended Altivec ABI ``` Comment at: clang/test/CodeGen/altivec.c:1 // RUN: %clang_cc1 -target-feature +altivec -triple powerpc-unknown-unknown -emit-llvm %s -o - | FileCheck %s +// RUN: %clang_cc1 -target-feature +altivec -mvecnvol -triple powerpc-unknown-aix -emit-llvm %s -o - | FileCheck %s 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`. Comment at: llvm/include/llvm/Target/TargetOptions.h:177 +/// volatile vector registers which is the default setting on AIX. +unsigned AIXExtendedAltivecABI = 0; + 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; ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88676/new/ https://reviews.llvm.org/D88676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88676: [PPC][AIX] Add vector callee saved registers for AIX extended vector ABI and add clang and llvm option
ZarkoCA marked 5 inline comments as done. ZarkoCA added inline comments. Comment at: llvm/test/CodeGen/PowerPC/aix-csr-vector.ll:4 +; RUN: FileCheck --check-prefix=MIR32 %s + +; RUN: llc -mtriple=powerpc-unknown-aix-xcoff -verify-machineinstrs \ Xiangling_L wrote: > 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? Yes, good point, I added that as well. Comment at: llvm/test/CodeGen/PowerPC/aix-csr-vector.ll:81 + +; ASM32: li {{[0-9]+}}, -192 +; ASM32-DAG: stxvd2x 52, 1, {{[0-9]+}} # 16-byte Folded Spill Xiangling_L wrote: > Xiangling_L wrote: > > Can we line up all comments? > 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. I'd rather not use any variables unless we need to use them later. Comment at: llvm/test/CodeGen/PowerPC/aix-csr-vector.ll:120 +; MIR32-LABEL: fixedStack: +; MIR32-NEXT:- { id: 0, type: spill-slot, offset: -144, size: 16, alignment: 16, stack-id: default, +; MIR32-NEXT:callee-saved-register: '$v31', callee-saved-restored: true, debug-info-variable: '', Xiangling_L wrote: > 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. Good suggestion, I added. Comment at: llvm/test/CodeGen/PowerPC/aix-csr-vector.ll:2 +; RUN: llc -mtriple=powerpc-unknown-aix-xcoff -verify-machineinstrs \ +; RUN: -mcpu=pwr7 -mattr=+altivec -stop-after=prologepilog < %s | \ +; RUN: FileCheck --check-prefix=MIR32 %s hubert.reinterpretcast wrote: > ZarkoCA wrote: > > Xiangling_L wrote: > > > ZarkoCA wrote: > > > > Xiangling_L wrote: > > > > > sfertile wrote: > > > > > > Minor nit: align this with the first argument in the preceeding > > > > > > line. > > > > > 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. > > > > 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. > > > I see. The only reason why I raise it up is because XL gives an error > > > when using altivec with unsupported arch. > > 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. > 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. > I added the options to toggle between the two Altivec ABIs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88676/new/ https://reviews.llvm.org/D88676 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D88676: [PPC][AIX] Add vector callee saved registers for AIX extended vector ABI and add clang and llvm option
ZarkoCA updated this revision to Diff 298409. 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. ZarkoCA added a comment. Herald added subscribers: cfe-commits, dang, dmgreen, arphaman. Herald added a project: clang. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88676/new/ https://reviews.llvm.org/D88676 Files: clang/docs/ClangCommandLineReference.rst clang/include/clang/Basic/CodeGenOptions.def clang/include/clang/Basic/DiagnosticDriverKinds.td clang/include/clang/Driver/Options.td clang/lib/CodeGen/BackendUtil.cpp clang/lib/Driver/ToolChains/Clang.cpp clang/lib/Frontend/CompilerInvocation.cpp clang/test/CodeGen/altivec.c llvm/include/llvm/CodeGen/CommandFlags.h llvm/include/llvm/Target/TargetMachine.h llvm/include/llvm/Target/TargetOptions.h llvm/lib/CodeGen/CommandFlags.cpp llvm/lib/Target/PowerPC/PPCCallingConv.td llvm/lib/Target/PowerPC/PPCFrameLowering.cpp llvm/lib/Target/PowerPC/PPCISelLowering.cpp llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp llvm/test/CodeGen/PowerPC/aix-AppendingLinkage.ll llvm/test/CodeGen/PowerPC/aix-csr-vector.ll llvm/test/CodeGen/PowerPC/aix-default-vec-abi.ll llvm/test/CodeGen/PowerPC/aix-func-align.ll llvm/test/CodeGen/PowerPC/aix-func-dsc-gen.ll llvm/test/CodeGen/PowerPC/aix-internal.ll llvm/test/CodeGen/PowerPC/aix-lower-block-address.ll llvm/test/CodeGen/PowerPC/aix-lower-constant-pool-index.ll llvm/test/CodeGen/PowerPC/aix-lower-jump-table.ll llvm/test/CodeGen/PowerPC/aix-reference-func-addr-const.ll llvm/test/CodeGen/PowerPC/aix-return55.ll llvm/test/CodeGen/PowerPC/aix-space.ll llvm/test/CodeGen/PowerPC/aix-xcoff-data-sections.ll llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-const.ll llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll llvm/test/CodeGen/PowerPC/aix-xcoff-reloc-large.ll llvm/test/CodeGen/PowerPC/aix-xcoff-textdisassembly.ll llvm/test/CodeGen/PowerPC/aix-xcoff-toc.ll llvm/test/CodeGen/PowerPC/aix32-crsave.mir llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll llvm/test/CodeGen/PowerPC/ppc32-i64-to-float-conv.ll llvm/test/CodeGen/PowerPC/ppc64-crsave.mir Index: llvm/test/CodeGen/PowerPC/ppc64-crsave.mir === --- llvm/test/CodeGen/PowerPC/ppc64-crsave.mir +++ llvm/test/CodeGen/PowerPC/ppc64-crsave.mir @@ -7,7 +7,7 @@ # RUN: FileCheck %s --check-prefixes=CHECK,SAVEALL -# RUN: llc -mtriple powerpc64-unknown-aix-xcoff -x mir -mcpu=pwr4 \ +# RUN: llc -mtriple powerpc64-unknown-aix-xcoff -x mir -mcpu=pwr4 -vecnvol \ # RUN: -run-pass=prologepilog --verify-machineinstrs < %s | \ # RUN: FileCheck %s --check-prefixes=CHECK,SAVEALL Index: llvm/test/CodeGen/PowerPC/ppc32-i64-to-float-conv.ll === --- llvm/test/CodeGen/PowerPC/ppc32-i64-to-float-conv.ll +++ llvm/test/CodeGen/PowerPC/ppc32-i64-to-float-conv.ll @@ -1,4 +1,4 @@ -; RUN: llc -verify-machineinstrs < %s -mcpu=pwr4 \ +; RUN: llc -verify-machineinstrs < %s -mcpu=pwr4 -vecnvol \ ; RUN: -mtriple=powerpc-ibm-aix-xcoff 2>&1 | FileCheck %s ; RUN: llc -verify-machineinstrs < %s -mcpu=pwr4 \ Index: llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll === --- llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll +++ llvm/test/CodeGen/PowerPC/lower-globaladdr64-aix-asm.ll @@ -1,7 +1,7 @@ -; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mtriple powerpc64-ibm-aix-xcoff \ +; RUN: llc -verify-machineinstrs -mcpu=pwr7 -vecnvol -mtriple powerpc64-ibm-aix-xcoff \ ; RUN: -code-model=small < %s | FileCheck %s --check-prefix=SMALL -; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mtriple powerpc64-ibm-aix-xcoff \ +; RUN: llc -verify-machineinstrs -mcpu=pwr7 -vecnvol -mtriple powerpc64-ibm-aix-xcoff \ ; RUN: -code-model=large < %s | FileCheck %s --check-prefix=LARGE @a = common global i32 0 Index: llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll === --- llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll +++ llvm/test/CodeGen/PowerPC/lower-globaladdr32-aix-asm.ll @@ -1,7 +1,7 @@ -; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mtriple powerpc-ibm-aix-xcoff \ +; RUN: llc -verify-machineinstrs -mcpu=pwr7 -vecnvol -mtriple powerpc-ibm-aix-xcoff \ ; RUN: -code-model=small < %s | FileCheck %s --check-prefix=SMALL -; RUN: llc -verify-machineinstrs -mcpu=pwr