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