peter.smith added a comment.
My main concern is that this changes the default behaviour for
arm-linux-gnueabi and arm-linux-gnueabihf targets. I've put some suggestions on
what I think the behaviour should be.
================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:277
+ }
+ return "";
+}
----------------
I'm a bit worried that we've changed the default behaviour for gnueabi[hf]
targets here.
For example with:
```
.text
vmov.32 d13[1], r6 ; Needs VFPv2
vadd.i32 d0, d0, d0 ; Needs Neon
```
I get with --target=armv7a-linux (no environment, -mfloat-abi will disable
floating point, and neon)
```
clang: warning: unknown platform, assuming -mfloat-abi=soft
neon.s:2:9: error: instruction requires: VFP2
vmov.32 d13[1],r6
^
neon.s:3:9: error: instruction requires: NEON
vadd.i32 d0, d0, d0
^
```
With the target=armv7a-linux-gnueabi armv7a-linux-gnueabihf or explicitly
adding -mfloat-abi=softfp the integrated assembler will happily assemble it.
GNU needs -mfpu=neon to assemble the file:
```
arm-linux-gnueabihf-as -march=armv7-a neon.s
neon.s: Assembler messages:
neon.s:2: Error: selected processor does not support ARM mode `vmov.32
d13[1],r6'
neon.s:3: Error: selected processor does not support ARM mode `vadd.i32
d0,d0,d0'
```
It is a similar story for armv8 and crypto.
I think we should have something like:
```
if (Triple.isLinux() && getARMSubArchVersionNumber(Triple) >= 8)
return "crypto-neon-fp-armv8";
if (Triple.isAndroid() || Triple.isLinux() &&
getARMSubArchVersionNumber(Triple) >= 7)
return "neon";
return "";
```
================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:389
+ std::string DefaultFPU = getDefaultFPUName(Triple);
+ if (DefaultFPU != "") {
+ if (!llvm::ARM::getFPUFeatures(llvm::ARM::parseFPU(DefaultFPU),
Features))
----------------
I'm wondering whether you need this bit of code anymore? In D53121 there needed
to be a switch between vfpv3-d16 and neon based on Android version. With
--target=armv7a-linux-android or --target=arm-linux-android -march=armv7a or
any v7a -mcpu applicable to Android then you'll get feature Neon by default and
won't need to do this? We could then move getDefaultFPUName out of ARM.cpp
================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:692
+ }
+ if (!hasMOrWaMArg(Args, options::OPT_mfpu_EQ, "-mfpu=")) {
+ std::string DefaultFPU = arm::getDefaultFPUName(Triple);
----------------
I think we'd not want to do this for -mfloat-abi=soft as this disables the FPU
in the integrated assembler. It seems like -mfloat-abi has no effect at all on
the gnu assembler, it will happily assemble neon instructions with
-mfloat-abi=soft -mfpu=neon.
================
Comment at: clang/test/Driver/linux-as.c:3
//
// RUN: %clang -target arm-linux -### \
// RUN: -no-integrated-as -c %s 2>&1 \
----------------
the target arm-linux (effectively arm-linux-unknown) defaults to
-mfloat-abi=soft which disables the FPU for the integrated assembler. While
these test cases are not wrong, the number of v7a + linux targets without an
FPU using entirely software floating point is likely to be very small. We
should have some more that have arm-linux-gnueabi and arm-linux-gnueabihf.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58314/new/
https://reviews.llvm.org/D58314
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
