[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) {
----------------
I am wondering what cases are not covered by `Triple.isOSAIX()`? Why do we also query `Triple.isOSBinFormatXCOFF()`?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4617
+      (Triple.isOSAIX() || Triple.isOSBinFormatXCOFF())) {
+    for (const Arg *A : Args) {
+      auto optID = A->getOption().getID();
----------------
I see your intention here is to find if there are any `-mabi=vec-extabi` or `-mabi=vec-default` option specifying. If I am correct, why we need to loop through whole Args? Can we just use `hasArg` query?


================
Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1417
+  if (Arg *A = 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
----------------
Based on the code added in `BackendUtil:551`, should we also add a case for compiling a source to assembly?


================
Comment at: llvm/include/llvm/Target/TargetMachine.h:246
 
+  bool getAIXExtendedAltivecABI() const {
+    return Options.AIXExtendedAltivecABI;
----------------
I don't see this function gets invoked anywhere,can we remove it?


================
Comment at: llvm/include/llvm/Target/TargetOptions.h:179
 
+    /// AIXExtendedAltivecABI - This flag returns true when -mabi=vec-extabi is
+    /// specified. The code generator is then able to use both volatile and
----------------
-mabi=vec-extabi is the FE option, should we s/-mabi=vec-extabi/-vec-extabi?


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:286
 
+  static cl::opt<bool> AIXExtendedAltivecABI(
+      "vec-extabi", cl::desc("Enable the AIX Extended Altivec ABI."),
----------------
Based on the naming convention in this file context, this seems should be `EnableAIXExtendedAltivecABI`.


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:287
+  static cl::opt<bool> AIXExtendedAltivecABI(
+      "vec-extabi", cl::desc("Enable the AIX Extended Altivec ABI."),
+      cl::init(false));
----------------
Can we add a testcase for this backend option?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-default-vec-abi.ll:1
+; RUN: not --crash llc < %s -mtriple powerpc64-ibm-aix-xcoff -mcpu=pwr8 2>&1 | FileCheck %s
+; RUN: not --crash llc < %s -mtriple powerpc-ibm-aix-xcoff -mcpu=pwr8 2>&1 | FileCheck %s
----------------
Can we simplify the testcase to only contain one vector type parameter? I think that would be sufficient to trigger the error.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-default-vec-abi.ll:4
+
+define dso_local <4 x i32> @vec_callee(<4 x i32> %vec1, <4 x i32> %vec2, <4 x i32> %vec3, <4 x i32> %vec4, <4 x i32> %vec5, <4 x i32> %vec6, <4 x i32> %vec7, <4 x i32> %vec8, <4 x i32> %vec9, <4 x i32> %vec10, <4 x i32> %vec11, <4 x i32> %vec12, <4 x i32> %vec13, <4 x i32> %vec14) {
+  entry:
----------------
ditto: can we simplify the testcase?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-xcoff-mergeable-str.ll:7
-; RUN: llc -verify-machineinstrs -mcpu=pwr4 \
-; RUN:     -mtriple powerpc-ibm-aix-xcoff  -data-sections=false < %s | FileCheck %s
-; RUN: llc -verify-machineinstrs -mcpu=pwr4 \
----------------
I am wondering why do we remove `-data-sections=false` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89684