[PATCH] D89684: [AIX] Add mabi=vec-extabi options to enable the AIX extended and default vector ABIs.

[Zarko Todorovski via Phabricator via cfe-commits]

ZarkoCA marked 5 inline comments as done.
ZarkoCA added inline comments.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1443
 
+  if (Arg *A =
+          Args.getLastArg(OPT_mabi_EQ_vec_default, OPT_mabi_EQ_vec_extabi)) {
----------------
Xiangling_L wrote:
> Should we also check if target feature altivec[`-target-feature +altivec`] is enabled when using these two options? If so, we should also add related testcases.
Both of these options require that -maltivec is also selected which sets `-target-feature +altivec`.


================
Comment at: clang/test/CodeGen/altivec.c:2
 // RUN: %clang_cc1 -target-feature +altivec -triple powerpc-unknown-unknown -emit-llvm %s -o - | FileCheck %s
-
+// RUN: %clang_cc1 -target-feature +altivec -mabi=vec-extabi -triple powerpc-unknown-aix -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -target-feature +altivec -mabi=vec-extabi -triple powerpc64-unknown-aix -emit-llvm %s -o - | FileCheck %s
----------------
Xiangling_L wrote:
> ZarkoCA wrote:
> > Xiangling_L wrote:
> > > Based on the code added in `BackendUtil:551`, should we also add a case for compiling a source to assembly?
> > Added in lines 15-18
> > Added in lines 15-18
> 
> Sorry, I should make my point clearer. Based on current testcases, there are two things:
> 
> 1. line 15-18 are actually duplication to 10,11, 13. 14. Because all of them are testing if the driver will emit error when not specifying -maltivec with -mabi=vec-default/-mabi=vec-extabi, i.e compiling from .c to .ll and .c to .s won't affect how driver works,
> 
> 2. `BackendUtil:551` The code I mentioned is actually affecting how BE behaves when we enable AIX altivec in the FE[or driver]. So the testcase I am looking for is something like:
> 
> ```
> // RUN:  %clang -target powerpc-unknown-aix -S -maltivec -mabi=vec-extabi %s  | FileCheck  %s
> // CHECK: LLVM ERROR: the extended Altivec AIX ABI is not yet supported
> ```
As far as I understand, testing the assembly path is a bit tricky mainly due to how Altivec is determined to be on for all powerpc targets.  

The tests in this file won't trigger the Altivec ABI errors because they are calling convention ABIs and there is no parameter passing or returns.  In fact, they will generate assembly because the default CPU has the Altivec attribute enabled.

I wrote tests that will use the Altivec calling convention ABI in those cases we  trigger earlier errors such as "vector type is unimplemented on AIX". 

But, I did a test which shows that the driver passes the `-mabi=vec-extabi` option. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-func-dsc-gen.ll:2
-; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mtriple powerpc-ibm-aix-xcoff -filetype=obj -o %t.o < %s
+; RUN: llc -verify-machineinstrs -mcpu=pwr7 -mattr=-altivec -mtriple powerpc-ibm-aix-xcoff -filetype=obj -o %t.o < %s
 ; RUN: llvm-readobj  --symbols %t.o | FileCheck %s
 
----------------
Xiangling_L wrote:
> I am not sure if this is for all testcases where you add `-mattr=-altivec`, but I tried the first three. They all passed without this option. Could you double check this?
You're right,it doesn't look like it's needed any longer. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89684/new/

https://reviews.llvm.org/D89684