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
