[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
