[ARM] Proper handling generic cpus

[vsukharev]

Summary

The patch mimics GCC's behaviour for the following:

• default -march is "armv4t"
• if an architecture is targeted, just a "generic" cpu is used with minimal sane set of features, instead of some specific real default cpu name.
• LLVM's specific exceptions:
  • for specific vendors, OSes, environments, or arches - add the minimally required features, or even pick a real cpu.
• build attributes for generic cpus:
  • asm: ".arch armv<ArchName>"
  • ELF: "Tag_CPU_name = "<ArchName>""
• default "-mfloat-abi" is "soft".
• if case of "-mfloat-abi=hard", and no "-mfpu" provided -> default "vfp2" is used.

Some revealed LLVM bugs had to be fixed along the patch preparation, because of test fails.

• Crypto feature did not include Neon feature. This was not visible, because Neon was always on.
• Despite requirements for minimal cpus were provided in Triple::getARMCPUForArch(), they did not always work.

Because of the latter fix and default arch upgrade, tests for codegen were also adjusted.

[rengolin]

lib/Basic/Targets.cpp

4253 ↗

(On Diff #31010)

nitpick: _ is a bit over. Call it "Kind", since it's obvious Kind of "what". :)

then:

ArchKind = Kind;

4351 ↗

(On Diff #31010)

Nice!

lib/Driver/Tools.cpp

729 ↗

(On Diff #31010)

This is odd. AAPCS_VFP != VFP.

It's not because we're using VFP PCS that we want to use VFP instructions in code and vice versa. I wouldn't assume it, at least.

test/Driver/arm-cortex-cpus.c

4 ↗

(On Diff #31010)

These tests are pretty much killed. If you're not testing for the default target-features in each one of them, you'll never know if you got the *real* "generic" one.

test/Driver/arm-mfpu.c

210 ↗

(On Diff #31010)

this is a better check than above...

test/Preprocessor/arm-target-features.c

5 ↗

(On Diff #31010)

I thought CRC was default ON.

18 ↗

(On Diff #31010)

This is one thing I disagree. Regardless of what GCC does, NEON in v7 is much more common than the alternative.

The bad thing about choosing the minimum requirement is that most people will just compile without the extra flags and will get a lot worse code out of it.

[t.p.northover]

test/Preprocessor/arm-target-features.c

18 ↗

(On Diff #31010)

I think it'd be a big shame not to have NEON by default on ARMv7 too. I don't think I could name a single v7 device without it.

[rengolin]

test/Preprocessor/arm-target-features.c

18 ↗

(On Diff #31010)

NVidia Tegra3 and Huawei D01s. Neither of them have a slice of the market big enough to matter.

[richard.barton.arm]

test/Preprocessor/arm-target-features.c

18 ↗

(On Diff #31010)

Isn't Vladimir's point philosophical though? His patch is suggesting that -march=armv7-a should give architectural ARMv7-A code, which would not include VFP or NEON as these are optional extensions.

[rengolin]

test/Preprocessor/arm-target-features.c

18 ↗

(On Diff #31010)

It is, and our stand is not what GCC normally does. Which is a shame on its own, but it's also a conscious decision.

As I said on the other thread, we do what most users expect will happen. The vast majority of v7A cores have NEON, VFP, hard-float. Choosing "-march=armv7a" will produce code that will run *well* on the majority of the cores it can target. If we did the bare minimum, it would run badly on *all* cores it can target.

We also like to burden the people that have specialised cores, not the vast majority that use all recommended standards. So, having to add -mfpu=vfp3 to Tegra3 instead of -mfpu=neon to everyone else, I think, is a good idea.

Distributions need a separate mechanism (PR20989) and changing the default will *not* help. Non-distro package builders only need to choose once what their defaults will be, so also not a strong argument.

[richard.barton.arm]

test/Preprocessor/arm-target-features.c

18 ↗

(On Diff #31010)

OK - I accept the current position and I don't want to turn this into a huge argument.

[rengolin]

Since this one depends on the LLVM counterpart that got rejected, and might need *a lot* of discussion about, I'll also mark this one as rejected.

The number of slight changes in this patch in architectures you can't control also give me confidence that this patch should not see the light of day unless all affected people have their say and agree with it.

test/Preprocessor/init.c

1975 ↗

(On Diff #31010)

Are you sure this is what they would expect?

[vsukharev]

Hi All, I have left only the absolute minimum.
General idea is to turn "generic" cpu name from real cpu name of armv8.1a (which was obviously wrong) to an abstract one.

[rengolin]

Hi Vladmir,

Please use "diff -U999" for better context. Comments inline.

cheers,
--renato

lib/Basic/Targets.cpp

4252 ↗

(On Diff #31361)

The more I look at that test, the more I'm convinced it's just plain wrong.

Please, remove the CPU of the equation and fix the test to emit the correct "default" arch, ARM_ARCH_4T.

4253 ↗

(On Diff #31361)

I don't get why we're setting the ArchInfo from CPUName, and up there we set Default CPU from ArchName.

4560 ↗

(On Diff #31361)

You already have information about the Arch in TargetInfo, you don't need the extra parameter.

Something like:

if (Name != "generic")
  setArchInfo(llvm::ARMTargetParser::parseCPUArch(Name));
if (ArchKind == llvm::ARM::AK_INVALID)
  return false;
...

would be enough.

[vsukharev]

lib/Basic/Targets.cpp

4252 ↗

(On Diff #31361)

Unfortunately, current default arch is not 4T, but arch of "arm1136j-s", because of initializer list of ARMTargetInfo::ARMTargetInfo().
If we would like to change it, it would be separate patch.
So I'll perform the changes that leave it intact.

[rengolin]

LGTM, thanks!

lib/Basic/Targets.cpp

4252 ↗

(On Diff #32435)

Ok, ignore this change for now. I'll think of a better fix, that probably will involve changing ARMTargetParser to emit "arm7tdmi" as the default for AK_INVALID.

[vsukharev]

Renato, that's weird enough, but the patch stops working after rebase, so I needed to add one more hunk.
No idea though, how it worked over 3-week ago sources... Anyway, it looks like should have been added even at that time, but tests were green...

[vsukharev]

lib/Driver/Tools.cpp

6007 ↗

(On Diff #32549)

That lines are changed after a rebase, due to test regression.

[rengolin]

3 weeks is a loooong time. :)

Code looks good, and if the tests pass, please commit.

cheers,
--renato

cfe/trunk/lib/Basic/Targets.cpp

class ARMTargetInfo : public TargetInfo {
   }
 
   void setArchInfo() {
     StringRef ArchName = getTriple().getArchName();
 
     ArchISA    = llvm::ARMTargetParser::parseArchISA(ArchName);
     DefaultCPU = getDefaultCPU(ArchName);
 
-    // SubArch is specified by the target triple
-    if (!DefaultCPU.empty()) 
-      setArchInfo(DefaultCPU);
-    else 
-      // FIXME ArchInfo should be based on ArchName from triple, not on 
-      // a hard-coded CPU name. Doing so currently causes regressions:
-      // test/Preprocessor/init.c: __ARM_ARCH_6J__ not defined
-      setArchInfo(CPU);
+    unsigned ArchKind = llvm::ARMTargetParser::parseArch(ArchName);
+    if (ArchKind == llvm::ARM::AK_INVALID)
+      // set arch of the CPU, either provided explicitly or hardcoded default
+      ArchKind = llvm::ARMTargetParser::parseCPUArch(CPU);
+    setArchInfo(ArchKind);
   }
 
-  void setArchInfo(StringRef CPU) {
+  void setArchInfo(unsigned Kind) {
     StringRef SubArch;
 
     // cache TargetParser info
-    ArchKind    = llvm::ARMTargetParser::parseCPUArch(CPU);
+    ArchKind    = Kind;
     SubArch     = llvm::ARMTargetParser::getSubArch(ArchKind);
     ArchProfile = llvm::ARMTargetParser::parseArchProfile(SubArch);
     ArchVersion = llvm::ARMTargetParser::parseArchVersion(SubArch);
 
     // cache CPU related strings
     CPUAttr    = getCPUAttr();
     CPUProfile = getCPUProfile();
   }
return llvm::StringSwitch<bool>(Feature)
         .Case("thumb", isThumb())
         .Case("neon", (FPU & NeonFPU) && !SoftFloat)
         .Case("hwdiv", HWDiv & HWDivThumb)
         .Case("hwdiv-arm", HWDiv & HWDivARM)
         .Default(false);
   }
 
   bool setCPU(const std::string &Name) override {
-    unsigned ArchKind = llvm::ARMTargetParser::parseCPUArch(Name);
+    if (Name != "generic")
+      setArchInfo(llvm::ARMTargetParser::parseCPUArch(Name));
+
     if (ArchKind == llvm::ARM::AK_INVALID)
       return false;
-    setArchInfo(Name);
     setAtomic();
     CPU = Name;
     return true;
   }
 
   bool setFPMath(StringRef Name) override;
 
   void getTargetDefines(const LangOptions &Opts,

cfe/trunk/lib/Driver/Tools.cpp

std::string arm::getARMTargetCPU(StringRef CPU, StringRef Arch,
 
   return getARMCPUForMArch(Arch, Triple);
 }
 
 /// getLLVMArchSuffixForARM - Get the LLVM arch name to use for a particular
 /// CPU  (or Arch, if CPU is generic).
 // FIXME: This is redundant with -mcpu, why does LLVM use this.
 const char *arm::getLLVMArchSuffixForARM(StringRef CPU, StringRef Arch) {
-  if (CPU == "generic" &&
-      llvm::ARMTargetParser::parseArch(Arch) == llvm::ARM::AK_ARMV8_1A)
-    return "v8.1a";
+  if (CPU == "generic")
+    return llvm::ARMTargetParser::getSubArch(
+        llvm::ARMTargetParser::parseArch(Arch));
 
   unsigned ArchKind = llvm::ARMTargetParser::parseCPUArch(CPU);
   if (ArchKind == llvm::ARM::AK_INVALID)
     return "";
   return llvm::ARMTargetParser::getSubArch(ArchKind);
 }
 
 void arm::appendEBLinkFlags(const ArgList &Args, ArgStringList &CmdArgs,

cfe/trunk/test/Driver/arm-cortex-cpus.c

 // RUN: %clang -target arm -march=armv8-a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A %s
 // RUN: %clang -target armv8 -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A %s
 // RUN: %clang -target arm -march=armv8 -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A %s
 // RUN: %clang -target armv8a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A %s
 // RUN: %clang -target arm -march=armv8a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A %s
 // RUN: %clang -target arm -mlittle-endian -march=armv8-a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A %s
 // CHECK-V8A: "-cc1"{{.*}} "-triple" "armv8-{{.*}}" "-target-cpu" "cortex-a53"
 
+// RUN: %clang -mcpu=generic -target armv8 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A-GENERIC %s
+// RUN: %clang -mcpu=generic -target arm -march=armv8 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A-GENERIC %s
+// RUN: %clang -mcpu=generic -target armv8a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A-GENERIC %s
+// RUN: %clang -mcpu=generic -target arm -march=armv8a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A-GENERIC %s
+// RUN: %clang -mcpu=generic -target arm -march=armv8-a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A-GENERIC %s
+// RUN: %clang -mcpu=generic -target armv8 -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A-GENERIC %s
+// RUN: %clang -mcpu=generic -target arm -march=armv8 -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A-GENERIC %s
+// RUN: %clang -mcpu=generic -target armv8a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A-GENERIC %s
+// RUN: %clang -mcpu=generic -target arm -march=armv8a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A-GENERIC %s
+// RUN: %clang -mcpu=generic -target arm -mlittle-endian -march=armv8-a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V8A-GENERIC %s
+// CHECK-V8A-GENERIC: "-cc1"{{.*}} "-triple" "armv8-{{.*}}" "-target-cpu" "generic"
+
 // RUN: %clang -target armebv8 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V8A %s
 // RUN: %clang -target armeb -march=armebv8 -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V8A %s
 // RUN: %clang -target armebv8a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V8A %s
 // RUN: %clang -target armeb -march=armebv8a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V8A %s
 // RUN: %clang -target armeb -march=armebv8-a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V8A %s
 // RUN: %clang -target armv8 -mbig-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V8A %s
 // RUN: %clang -target arm -march=armebv8 -mbig-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V8A %s
 // RUN: %clang -target armv8a -mbig-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V8A %s

 
 // RUN: %clang -target arm -march=armv8.1a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
 // RUN: %clang -target armv8.1a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
 // RUN: %clang -target arm -march=armv8.1-a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
 // RUN: %clang -target arm -march=armv8.1a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
 // RUN: %clang -target armv8.1a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
 // RUN: %clang -target arm -march=armv8.1a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
 // RUN: %clang -target arm -mlittle-endian -march=armv8.1-a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
+// RUN: %clang -mcpu=generic -target arm -march=armv8.1a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
+// RUN: %clang -mcpu=generic -target armv8.1a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
+// RUN: %clang -mcpu=generic -target arm -march=armv8.1-a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
+// RUN: %clang -mcpu=generic -target arm -march=armv8.1a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
+// RUN: %clang -mcpu=generic -target armv8.1a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
+// RUN: %clang -mcpu=generic -target arm -march=armv8.1a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
+// RUN: %clang -mcpu=generic -target arm -mlittle-endian -march=armv8.1-a -mlittle-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-V81A %s
 // CHECK-V81A: "-cc1"{{.*}} "-triple" "armv8.1a-{{.*}}" "-target-cpu" "generic" "-target-feature" "+v8.1a"
 
 // RUN: %clang -target armebv8.1a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V81A %s
 // RUN: %clang -target armeb -march=armebv8.1a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V81A %s
 // RUN: %clang -target armeb -march=armebv8.1-a -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V81A %s
 // RUN: %clang -target armv8.1a -mbig-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V81A %s
 // RUN: %clang -target arm -march=armebv8.1a -mbig-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V81A %s
 // RUN: %clang -target arm -march=armebv8.1-a -mbig-endian -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-BE-V81A %s