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

Thu Feb 21 09:41:48 PST 2019

peter.smith added a comment.

Thanks for the update. To summarise I'd like to keep the integrated and non-integrated assembler in synch for Linux like Android, even at the risk of adding an fpu when it isn't needed. I think that given how difficult it is to not use NEON, I think the mfloat-abi=soft guard you've put on will be sufficient. By the way, I'm hoping we aren't talking past each other with the default for armv7-a. I've tried to put as much of what I understand in the comments and I hope the answers make sense, or you'll be able to correct me where I'm wrong. If the timezone delayed comments aren't working well it may be worth finding me on IRC, I usually leave the office about 6:30pm but I can easily arrange a time and log on later if you wanted to discuss.

I also spotted something else on line 251 of  ARM.cpp with respect to Android that might be worth looking at. I've left a comment although it isn't related to this patch.

================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:277
+  }
+  return "";
+}
----------------
danalbert wrote:
> 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.
> 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 is true that LLVM and GNU have unfortunately chosen a different default for armv7-a and NEON and armv8-a and crypo. I agree that NEON is not an assumed feature of either a gnueabi or gnueabihf GCC toolchain. My understanding is that GNU chose to not include any extensions in the base architecture and LLVM chose the most common configuration for the default.

>  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.

Are you sure that we have no FPU by default with the integrated assembler for armv7-a and armv8-a? I've put an explanation in the next comment that explains how it gets set even when there is no +neon on the -cc1 or -cc1as command line. 

> 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.

My personal view is that "clang -fintegrated-as <some options implying armv7-a or above> neon.s" should give the same result as "clang -fno-integrated-as <some options implying armv7-a or above> neon.s". My reasoning is that is the least surprising position for users, and doesn't change old behaviour. That view is arguable and the opposite is also logically consistent, maybe worth reaching out to some more reviewers to see what they think. 

Right now I can't even find a way to disable neon in the integrated assembler apart from -mfloat-abi=soft so I'm not overly concerned about passing -mfpu=neon to GNU-as. FWIW the only link I could find for people not wanting neon was https://github.com/ldc-developers/ldc/issues/1752 

Eventually when we support the recent gcc -march=name[+extension…] format in https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html then we'll need to revisit this.

================
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))
----------------
danalbert wrote:
> 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.
My understanding is that adding the equivalent of -fpu=neon here has the effect of adding extra feature flags to the call to clang -cc1. I can observe the difference with -###. However when compiling these are then added back in by TargetInfo::CreateTargetInfo() by calls like initFeatureMap that take the default features from the target. When invoking -cc1as the createMCSubtargetInfo via a long chain of function calls will add the FeatureNEON bit for armv7-a unless the -neon feature has been cleared.  

If I have it right I think the lack of floating point features in the clang -cc1 command line is not a problem as the defaults for armv7-a will put them in anyway unless they've been explicitly turned off via something like -mfloat-abi=soft. My information is based on reverse engineering the code so I could easily be missing something important though?

================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:251
       case llvm::Triple::Android:
         ABI = (SubArch == 7) ? FloatABI::SoftFP : FloatABI::Soft;
         break;
----------------
Not strictly related to this patch, but you'll probably want to make that (SubArch >= 7), I tried --target=armv8a-linux-android as an experiment and got my floating point support removed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58314