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

Thu Feb 21 12:53:28 PST 2019

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