[PATCH] D89684: [AIX] Add mvecnvol and mnovecnvol 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/docs/ClangCommandLineReference.rst:2868
 
+Only supported On AIX. Specify usage of volatile and nonvolatile vector registers, the extended vector ABI on AIX. Defaults to '-mnovecnvol' when Altivec is enabled.
+
----------------
s/On/on;



================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:532
   Options.EmitCallSiteInfo = CodeGenOpts.EmitCallSiteInfo;
+  Options.AIXExtendedAltivecABI = CodeGenOpts.AIXExtendedAltivecABI;
   Options.ValueTrackingVariableLocations =
----------------
The ABI specifies `When the option to use nonvolatile vector registers is enalbed. the compilation environment must also predefine __EXTABI__`. I didn't see this. Should we also cover this in this patch?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4571
   }
 
+  if (Arg *A =
----------------
On clang, when we do: 
`clang -target powerpc-ibm-aix-xcoff -maltivec -S -emit-llvm test_faltivec.c`,  clang driver passes `-target-cpu pwr4` as default arch to frontend without issuing any error.

However, with XL, we have: 
`"-qaltivec" is not compatible with "-qarch=pwr4". "-qnoaltivec" is being set.`  The same error will be issued if `pwr5` is used as well. 

So I suppose for AIX in clang, when user use `-maltivec` without specifying arch level, we can do:
1)  by default pass `-target-cpu pwr6` to frontend 
or  2) issue error for "-qarch=pwr4"+ enable altivec
or 3) issue error for `-qacrh = pwr4` + diable altivec like XL does?

Also we should emit error when user use `-maltivec` with -mcpu=pwr5.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4579
+
+    bool haveMaltivec = false;
+
----------------
I would suggest `s/haveMaltivec/HasAltivec` to be consistent with other places where if altivec enabled is tested.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4581
+
+    for (const Arg *A : Args) {
+      auto optID = A->getOption().getID();
----------------
Any reason why we cannot use `Args.getLastArg` here for `OPT_maltivec` as well?


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4591
+
+    if (A->getOption().matches(options::OPT_mnovecnvol) && haveMaltivec)
+      D.Diag(diag::err_aix_default_altivec_abi);
----------------
Since we are defaulting to default altivec ABI, so I think the logic here should be if (HasAltivec && !Args.getLastArg(options::OPT_mvecnvol)), then we emit `D.Diag(diag::err_aix_default_altivec_abi)` error?


================
Comment at: clang/test/CodeGen/altivec.c:53
 }
+
+// AIX-ERROR:  error: The default Altivec ABI on AIX is not yet supported, use '-mvecnvol' for the extended Altivec ABI
----------------
Could we also add a testcase to test `-mvecnvol/-mnovecnvol` are AIX only options?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89684