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

[Dan Albert via Phabricator via cfe-commits]

danalbert marked 3 inline comments as done.
danalbert added inline comments.



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



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:251
   case llvm::Triple::Android:
 ABI = (SubArch == 7) ? FloatABI::SoftFP : FloatABI::Soft;
 break;

peter.smith wrote:
> 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.
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.


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

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

[Peter Smith via Phabricator via cfe-commits]

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 
> 

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dan Albert via Phabricator via cfe-commits]

danalbert updated this revision to Diff 187670.
danalbert marked 6 inline comments as done.
danalbert added a comment.

Updated to address some review comments:

- Additional tests for gnueabi/gnueabihf
- Fixed behavior for `-mfpu=neon -mfloat-abi=soft`, added test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58314

Files:
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.h
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/linux-as.c

Index: clang/test/Driver/linux-as.c
===
--- clang/test/Driver/linux-as.c
+++ clang/test/Driver/linux-as.c
@@ -3,17 +3,17 @@
 // RUN: %clang -target arm-linux -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM %s
-// CHECK-ARM: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft"
+// CHECK-ARM: as{{(.exe)?}}" "-EL" "-march=armv4t" "-mfloat-abi=soft"
 //
 // RUN: %clang -target arm-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MCPU %s
-// CHECK-ARM-MCPU: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-ARM-MCPU: as{{(.exe)?}}" "-EL" "-march=armv7" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target arm-linux -mfpu=neon -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MFPU %s
-// CHECK-ARM-MFPU: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-mfpu=neon"
+// CHECK-ARM-MFPU: as{{(.exe)?}}" "-EL" "-march=armv4t" "-mfloat-abi=soft" "-mfpu=neon"
 //
 // RUN: %clang -target arm-linux -march=armv7-a -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
@@ -63,62 +63,97 @@
 // RUN: %clang -target armv7-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-TARGET %s
-// CHECK-ARM-TARGET: as{{(.exe)?}}" "-EL" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-ARM-TARGET: as{{(.exe)?}}" "-EL" "-march=armv7" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target armebv7-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARMEB-TARGET %s
-// CHECK-ARMEB-TARGET: as{{(.exe)?}}" "-EB" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-ARMEB-TARGET: as{{(.exe)?}}" "-EB" "-march=armebv7" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target thumbv7-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMB-TARGET %s
-// CHECK-THUMB-TARGET: as{{(.exe)?}}" "-EL" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-THUMB-TARGET: as{{(.exe)?}}" "-EL" "-march=armv7" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target thumbebv7-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMBEB-TARGET %s
-// CHECK-THUMBEB-TARGET: as{{(.exe)?}}" "-EB" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-THUMBEB-TARGET: as{{(.exe)?}}" "-EB" "-march=armebv7" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target armv8-linux -mcpu=cortex-a53 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-TARGET-V8 %s
-// CHECK-ARM-TARGET-V8: as{{(.exe)?}}" "-EL" "-mfpu=crypto-neon-fp-armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
+// CHECK-ARM-TARGET-V8: as{{(.exe)?}}" "-EL" "-march=armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
 //
 // RUN: %clang -target armebv8-linux -mcpu=cortex-a53 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARMEB-TARGET-V8 %s
-// CHECK-ARMEB-TARGET-V8: as{{(.exe)?}}" "-EB" "-mfpu=crypto-neon-fp-armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
+// CHECK-ARMEB-TARGET-V8: as{{(.exe)?}}" "-EB" "-march=armebv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
 //
 // RUN: %clang -target thumbv8-linux -mcpu=cortex-a53 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMB-TARGET-V8 %s
-// CHECK-THUMB-TARGET-V8: as{{(.exe)?}}" "-EL" "-mfpu=crypto-neon-fp-armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
+// CHECK-THUMB-TARGET-V8: as{{(.exe)?}}" "-EL" "-march=armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
 //
 // RUN: %clang -target thumbebv8-linux -mcpu=cortex-a53 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMBEB-TARGET-V8 %s
-// CHECK-THUMBEB-TARGET-V8: as{{(.exe)?}}" "-EB" "-mfpu=crypto-neon-fp-armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
+// CHECK-THUMBEB-TARGET-V8: as{{(.exe)?}}" "-EB" "-march=armebv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
 //
 // RUN: %clang -target arm-linux -mfloat-abi=hard -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MFLOAT-ABI %s
-// CHECK-ARM-MFLOAT-ABI: as{{(.exe)?}}" "-EL" "-mfloat-abi=hard"
+// 

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dan Albert via Phabricator via cfe-commits]

danalbert created this revision.
danalbert added reviewers: peter.smith, kristof.beyls, srhines, pirama.
Herald added a subscriber: javed.absar.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58314

Files:
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.h
  clang/lib/Driver/ToolChains/Gnu.cpp
  clang/test/Driver/linux-as.c

Index: clang/test/Driver/linux-as.c
===
--- clang/test/Driver/linux-as.c
+++ clang/test/Driver/linux-as.c
@@ -3,17 +3,17 @@
 // RUN: %clang -target arm-linux -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM %s
-// CHECK-ARM: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft"
+// CHECK-ARM: as{{(.exe)?}}" "-EL" "-march=armv4t" "-mfloat-abi=soft"
 //
 // RUN: %clang -target arm-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MCPU %s
-// CHECK-ARM-MCPU: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-ARM-MCPU: as{{(.exe)?}}" "-EL" "-march=armv7" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target arm-linux -mfpu=neon -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-MFPU %s
-// CHECK-ARM-MFPU: as{{(.exe)?}}" "-EL" "-mfloat-abi=soft" "-mfpu=neon"
+// CHECK-ARM-MFPU: as{{(.exe)?}}" "-EL" "-march=armv4t" "-mfloat-abi=soft" "-mfpu=neon"
 //
 // RUN: %clang -target arm-linux -march=armv7-a -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
@@ -63,62 +63,72 @@
 // RUN: %clang -target armv7-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-TARGET %s
-// CHECK-ARM-TARGET: as{{(.exe)?}}" "-EL" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-ARM-TARGET: as{{(.exe)?}}" "-EL" "-march=armv7" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target armebv7-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARMEB-TARGET %s
-// CHECK-ARMEB-TARGET: as{{(.exe)?}}" "-EB" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-ARMEB-TARGET: as{{(.exe)?}}" "-EB" "-march=armebv7" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target thumbv7-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMB-TARGET %s
-// CHECK-THUMB-TARGET: as{{(.exe)?}}" "-EL" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-THUMB-TARGET: as{{(.exe)?}}" "-EL" "-march=armv7" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target thumbebv7-linux -mcpu=cortex-a8 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMBEB-TARGET %s
-// CHECK-THUMBEB-TARGET: as{{(.exe)?}}" "-EB" "-mfpu=neon" "-mfloat-abi=soft" "-mcpu=cortex-a8"
+// CHECK-THUMBEB-TARGET: as{{(.exe)?}}" "-EB" "-march=armebv7" "-mfloat-abi=soft" "-mcpu=cortex-a8"
 //
 // RUN: %clang -target armv8-linux -mcpu=cortex-a53 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARM-TARGET-V8 %s
-// CHECK-ARM-TARGET-V8: as{{(.exe)?}}" "-EL" "-mfpu=crypto-neon-fp-armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
+// CHECK-ARM-TARGET-V8: as{{(.exe)?}}" "-EL" "-march=armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
 //
 // RUN: %clang -target armebv8-linux -mcpu=cortex-a53 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-ARMEB-TARGET-V8 %s
-// CHECK-ARMEB-TARGET-V8: as{{(.exe)?}}" "-EB" "-mfpu=crypto-neon-fp-armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
+// CHECK-ARMEB-TARGET-V8: as{{(.exe)?}}" "-EB" "-march=armebv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
 //
 // RUN: %clang -target thumbv8-linux -mcpu=cortex-a53 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck -check-prefix=CHECK-THUMB-TARGET-V8 %s
-// CHECK-THUMB-TARGET-V8: as{{(.exe)?}}" "-EL" "-mfpu=crypto-neon-fp-armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
+// CHECK-THUMB-TARGET-V8: as{{(.exe)?}}" "-EL" "-march=armv8" "-mfloat-abi=soft" "-mcpu=cortex-a53"
 //
 // RUN: %clang -target thumbebv8-linux -mcpu=cortex-a53 -### \
 // RUN:   -no-integrated-as -c %s 2>&1 \
 // RUN:   | FileCheck