Page MenuHomePhabricator

[Driver] Sync ARM behavior between clang-as and gas.
Needs ReviewPublic

Authored by danalbert on Feb 15 2019, 4:58 PM.

Details

Summary

The ARM gas driver previously enabled NEON for ARMv7 and up, and a
handful of other extensions for ARMv8 and up (although only if the
architecture version was a part of the triple; the -march flag was
not honored). Neither of these are actually guaranteed, and the
behavior does not match the integrated assembler.

Note that I've elected to always pass an explicit -march flag to gas
if the user has not specified one. I'm not certain if Clang's default
ARM version matches GNU's, so being explicit allows us to keep the
same behavior if that isn't the case. This also makes it clear that
the correct architecture is passed to gas based on -mcpu.

Event Timeline

danalbert created this revision.Feb 15 2019, 4:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 4:58 PM
Herald added a subscriber: javed.absar. · View Herald Transcript

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.

clang/lib/Driver/ToolChains/Arch/ARM.cpp
277

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 "";
389

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

clang/lib/Driver/ToolChains/Gnu.cpp
692

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.

clang/test/Driver/linux-as.c
3

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.

danalbert updated this revision to Diff 187670.Feb 20 2019, 1:56 PM
danalbert marked 6 inline comments as done.

Updated to address some review comments:

  • Additional tests for gnueabi/gnueabihf
  • Fixed behavior for -mfpu=neon -mfloat-abi=soft, added test.
danalbert added inline comments.Feb 20 2019, 1:56 PM
clang/lib/Driver/ToolChains/Arch/ARM.cpp
277

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.

389

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.

clang/lib/Driver/ToolChains/Gnu.cpp
692

Added a check for soft float ABI and added a test to match.

clang/test/Driver/linux-as.c
3

Added a handful. Not really sure what needs to be tested here, but I covered the basic cases.

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.

clang/lib/Driver/ToolChains/Arch/ARM.cpp
251

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.

277

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.

389

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?

danalbert marked 3 inline comments as done.Feb 21 2019, 12:52 PM
danalbert added inline comments.
clang/lib/Driver/ToolChains/Arch/ARM.cpp
251

I actually uploaded a fix for that yesterday: https://reviews.llvm.org/D58477

Didn't submit because I wanted to wait until I knew I'd be around to babysit build bots just in case.

389

Oh, I thought that the cc1 args were the full story. I'll need to do some more digging then to make sure that the behavior actually does match.