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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4616
+  if (Args.hasArg(options::OPT_maltivec) &&
+      (Triple.isOSAIX() || Triple.isOSBinFormatXCOFF())) {
+    for (const Arg *A : Args) {
----------------
ZarkoCA wrote:
> Xiangling_L wrote:
> > I am wondering what cases are not covered by `Triple.isOSAIX()`? Why do we also query `Triple.isOSBinFormatXCOFF()`?
> The path isn't selected if someone were to select -powerpc-unknown-xcoff as a target for example.  It looks like the Triple.isOSAIX() is true when we we have aix in the target triple. 
> The path isn't selected if someone were to select -powerpc-unknown-xcoff as a target for example.  It looks like the Triple.isOSAIX() is true when we we have aix in the target triple. 

My understanding is that the AIX altivec ABI is target-dependent, it has nothing to do with binary format.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4617
+
+    if (Args.hasArg(options::OPT_maltivec) &&
+        (Args.hasArg(options::OPT_mabi_EQ_vec_extabi))) {
----------------
The common query `Args.hasArg(options::OPT_maltivec)` is better to put in an upper level `if`;
Also after you do that, the last `else if` can be replaced by a `else`;
Another issue is that when we on AIX with -maltivec + extended/defautlt altivec abi enabled, we do duplicate checking in #4615 & #4613 `if` blocks. 

Maybe we can refactor these two `if` blocks into:

```
  if (Arg *A = Args.getLastArg(options::OPT_mabi_EQ_vec_extabi,
                               options::OPT_mabi_EQ_vec_default)) {
    if (!Triple.isOSAIX()) {
      D.Diag(diag::err_drv_unsupported_opt_for_target)
          << A->getSpelling() << RawTriple.str();
    } else {
      if (Args.hasArg(options::OPT_maltivec)) {
        if (Args.hasArg(options::OPT_mabi_EQ_vec_extabi)) {
          CmdArgs.push_back("-mabi=vec-extabi");
        } else if (Args.hasArg(options::OPT_mabi_EQ_vec_default)) {
          D.Diag(diag::err_aix_default_altivec_abi);
        } else {
          D.Diag(diag::err_aix_default_altivec_abi);
        }
      } else {
        D.Diag(diag::err_aix_altivec);
      }
    }
  }
```	


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4633
+                               options::OPT_mabi_EQ_vec_default)) {
+    if (!Triple.isOSAIX() || !Triple.isOSBinFormatXCOFF())
+      D.Diag(diag::err_drv_unsupported_opt_for_target)
----------------
ditto.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1443
 
+  if (Arg *A =
+          Args.getLastArg(OPT_mabi_EQ_vec_default, OPT_mabi_EQ_vec_extabi)) {
----------------
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.


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1445
+          Args.getLastArg(OPT_mabi_EQ_vec_default, OPT_mabi_EQ_vec_extabi)) {
+    if (!T.isOSAIX() || !T.isOSBinFormatXCOFF())
+      Diags.Report(diag::err_drv_unsupported_opt_for_target)
----------------
ditto.


================
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
----------------
It looks line 2& 4, line 3&5 are duplicated.


================
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
----------------
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
```


================
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
 
----------------
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?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-vec-abi.ll:1
+; RUN: not --crash llc < %s -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr8 2>&1 | FileCheck %s --check-prefix=DFLTERROR
+; RUN: not --crash llc < %s -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr8 2>&1 | FileCheck %s --check-prefix=DFLTERROR
----------------
May I ask why we use `pwr8` for this test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89684