[PATCH] D88676: [PPC][AIX] Add vector callee saved registers for AIX extended vector ABI and add clang and llvm option

[Zarko Todorovski via Phabricator via cfe-commits]

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