[PATCH] D89684: [AIX] Add mabi=vec-extabi options to enable the AIX extended and default vector ABIs.

[Zarko Todorovski via Phabricator via llvm-commits]

ZarkoCA marked 8 inline comments as done.
ZarkoCA added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:532
   Options.EmitCallSiteInfo = CodeGenOpts.EmitCallSiteInfo;
+  Options.AIXExtendedAltivecABI = CodeGenOpts.AIXExtendedAltivecABI;
   Options.ValueTrackingVariableLocations =
----------------
Xiangling_L wrote:
> 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?
Thanks, that was an oversight. 


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4571
   }
 
+  if (Arg *A =
----------------
Xiangling_L wrote:
> 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.
I think what XL does is probably the correct thing but in clang/llvm it looks like the hasAltivec setting is determined by the cpu level and the compiler simply ignores it when it's not supported by the cpu.  

For now, I'd like to follow the existing logic as all the other PPC targets and then I can follow up with a patch that emits an error when selecting altivec when the cpu doesn't support it.   


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4579
+
+    bool haveMaltivec = false;
+
----------------
Xiangling_L wrote:
> I would suggest `s/haveMaltivec/HasAltivec` to be consistent with other places where if altivec enabled is tested.
I reworked this so that I hopefully remove any confusion. 


================
Comment at: llvm/lib/CodeGen/CommandFlags.cpp:489
     Options.FloatABIType = getFloatABIForCalls();
+  Options.AIXExtendedAltivecABI = getAIXExtendedAltivecABI();
   Options.NoZerosInBSS = getDontPlaceZerosInBSS();
----------------
Xiangling_L wrote:
> Should we also check `-vecnvol` option is used for AIX only somewhere?
Is there a way to check whether an llc option is target specific?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-AppendingLinkage.ll:4
 
-; RUN: llc -verify-machineinstrs -mcpu=pwr4 -mtriple powerpc64-ibm-aix-xcoff < \
+; RUN: llc -verify-machineinstrs -mcpu=pwr4 -vecnvol -mtriple powerpc64-ibm-aix-xcoff < \
 ; RUN: %s | FileCheck %s
----------------
Xiangling_L wrote:
> May I ask why would we want to add -vecnvol for those testcases? As I noticed, they don't need altivec feature enabled.
It is odd but those test cases hit the error `llvm/lib/Target/PowerPC/PPCISelLowering.cpp:6908` when that wasn't enabled.  However, adding `mattr=-altivec` also suppresses it. It seems like specifying `mcpu=pwr4` doesn't not completely remove all altivec opts? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89684