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