[PATCH] D58314: [Driver] Sync ARM behavior between clang-as and gas.

[Peter Smith via Phabricator via cfe-commits]

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