[PATCH] D88676: [PPC][AIX] Add vector callee saved registers for AIX extended vector ABI and add clang and llvm option

[Xiangling Liao via Phabricator via llvm-commits]

Xiangling_L added a comment.

I am wondering can we split the option related changes to a separate patch for reviews? That would make current patch a bit easier to review and faster to be committed as two small pieces.

If it's possible, I am thinking we can try to split it up to the following two pieces:

1. Add option in the frontend and backend to be able to turn on extended vector ABI
2. Do the frame lowing in the backend



================
Comment at: clang/docs/ClangCommandLineReference.rst:2868
 
+Specify usage of volatile and nonvolatile vector registers, the extended vector ABI on AIX (AIX only).  The default AIX vector ABI is not yet supported. 
+
----------------
1. I am not sure if it's a good idea to put the supporting status also in the option description here. It looks a bit strange to me.

2. I would suggest something similar like this for the option description:


```
Only supported on AIX. Specifies whether to use both volatile and nonvolatile vector registers or volatile vector registers only. Defaults to `-mnovecnvol` when Altivec is enabled. 
```

3. We missed a `-` before `mnovecnvol`.


================
Comment at: clang/include/clang/Basic/DiagnosticDriverKinds.td:531
+def err_aix_default_altivec_abi : Error<
+  "The default Altivec ABI on AIX is not yet supported, use the extended ABI option '-mvecnvol'">;
+
----------------
I would suggest:

```
The default Altivec ABI on AIX is not yet supported, use '-mvecnvol' for the extended Altivec ABI 
```


================
Comment at: clang/test/CodeGen/altivec.c:1
 // RUN: %clang_cc1 -target-feature +altivec -triple powerpc-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1 -target-feature +altivec -mvecnvol -triple powerpc-unknown-aix -emit-llvm %s -o - | FileCheck %s
----------------
Can we also test how the driver react to these two options? It would serve as the LIT coverage for the code change in `clang/lib/Driver/ToolChains/Clang.cpp`.


================
Comment at: llvm/include/llvm/Target/TargetOptions.h:177
+    /// volatile vector registers which is the default setting on AIX.
+    unsigned AIXExtendedAltivecABI = 0;
+
----------------
Can we also use bitfield to indicate true and false here? The default value set to be `false` in ctor already, so we don't need assign `0` to it here.

```
unsigned AIXExtendedAltivecABI : 1;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88676