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

[Dan Albert via Phabricator via cfe-commits]

danalbert added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:277
+  }
+  return "";
+}
----------------
peter.smith wrote:
> 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 "";
> ```
I suppose it depends on which direction you want the behavior change to go. I assumed those samples _shouldn't_ assemble since they're not enabling NEON. The fact that the direct `arm-linux-gnueabihf-as` doesn't enable NEON by default makes me assume that NEON is not an assumed feature of the gnueabihf environment.

It's not up to me whether NEON is available by default for ARMv7 for non-Android, but I do think that the behavior should be consistent regardless of the assembler being used. Right now we have no FPU by default with the integrated assembler and NEON by default with GAS. This change makes GAS have the same behavior as the integrated assembler, since I assume that is the better traveled path and afaict is the one that has had more effort put in to it.

If that's not right, I can change this so that the integrated assembler _also_ gets FPU features that are not necessarily available for the given architecture, but I wanted to clarify that that is what you're asking for first.


================
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))
----------------
peter.smith wrote:
> 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
I don't understand. This bit of code is the piece that provides the NEON default. If I remove this then Android ARMv7 targets revert to no FPU features.


================
Comment at: clang/lib/Driver/ToolChains/Gnu.cpp:692
+    }
+    if (!hasMOrWaMArg(Args, options::OPT_mfpu_EQ, "-mfpu=")) {
+        std::string DefaultFPU = arm::getDefaultFPUName(Triple);
----------------
peter.smith wrote:
> 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.
>  
Added a check for soft float ABI and added a test to match.


================
Comment at: clang/test/Driver/linux-as.c:3
 //
 // RUN: %clang -target arm-linux -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
----------------
peter.smith wrote:
> 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.
Added a handful. Not really sure what needs to be tested here, but I covered the basic cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58314