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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
