[ARM] Compute a target feature which corresponds to the ARM version.

[efriedma]

Summary

Currently, the interaction between the triple, the CPU, and the supported features is a mess: the driver edits the triple to indicate the supported architecture version, and the LLVM backend uses this to figure out what instructions are legal. This makes it difficult to understand what's happening, and makes it impossible to LTO together two modules with different computed architectures.

Instead of relying on triple rewriting to get the correct target features, we should add the right target features explicitly.

[SjoerdMeijer]

Agree, this is a real mess.
This change looks good to me.

Was only wondering, we are only testing target features +v7 and +v8, but do we now also need to check the others, like +v8.1a, +v8.2a, etc.?

[fhahn]

Thanks for looking into this Eli! Adding the architecture version as target feature looks good to me.

lib/Basic/Targets/ARM.cpp

342

Would it make sense to add this to llvm/Support/TargetParser? We already have AArch64::getArchFeatures there and it looks like it does something similar for AArch64.

llvm/Support/ARMTargetParser.def defines the different architectures + extensions and it seems like it should possible to extend it slightly to handle the architecture features.

[fhahn]

We also add thumb-mode to the target features, for a similar reason, allowing mixed Thumb/Arm codegen with LTO.

[efriedma]

Fixed to use the arch name as the "feature"; otherwise, we miss relevant features in certain cases. This is taken from ARMAsmParser::parseDirectiveArch.

[fhahn]

lib/Basic/Targets/ARM.cpp

409

Is there a reason we re-compute the Arch from the triple here, but use the member variable ArchKind above? Shouldn't they both be the same ?

[efriedma]

lib/Basic/Targets/ARM.cpp

409

ArchKind is based on the triple and the CPU, where "Arch" is just based on the triple, so "Arch" was returning the wrong thing in some cases.

Actually, looking a bit more, we should probably be calling parseCPUArch here to get the right behavior, rather than depending on the member. (It doesn't usually matter, but it could make a difference for code using "target" attribute, I guess. That feature needs a lot more work, though.)

[efriedma]

Compute arch used passed-in CPU, not the global arch.

[rengolin]

LTO is something I never considered before in the context of the target parser, but I understand the issues are similar to what the build attributes were trying to solve, so adding more info shouldn't make it worse.

However, this will probably break obscure cases that normally only @compnerd and @joerg pick up. I, personally, don't have any opinion on how much this makes sense or not. :/

I'm with @fhahn that we should make ARM and AArch64 as common as possible, not only in code for the target parser but also in behaviour for the front-end, mostly because ARMv8 spans both and we want to avoid surprises.

[joerg]

Can you make sure that we handle the older ARM versions correctly as well, i.e. v4, v5 and v6? I take it we still have test cases for the arm <-> thumb transition? That's the one part of the triple logic that is really non-trivial.

[fhahn]

Thanks for updating it to use the stuff from TargetParser.

LGTM, with tests for the cases @joerg mentiond.

lib/Basic/Targets/ARM.cpp

409

Thanks that makes sense. getDefaultFPU and getDefaultExtensions are only for "generic" FPUs.

[efriedma]

Add a bunch of tests for various arm arches. Make sure we do something reasonable when we can't figure out any arch at all.

[efriedma]

I take it we still have test cases for the arm <-> thumb transition?

"-mthumb" and "-marm" are handled in the driver by rewriting the triple.

[stefson]

hey there, I've run into problems with stripping static-libs on arm when using llvm/clang-7. Could you imagine this patch being at fault?

strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneeded -R .comment -R .GCC.command.line -R .note.gnu.gold-version
   lib/libbz2.so.1.0.6
   usr/bin/bzip2recover
   bin/bzip2
   usr/lib/libbz2.a
armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11
armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11

[peter.smith]

In D45240#1267846, @stefson wrote:

hey there, I've run into problems with stripping static-libs on arm when using llvm/clang-7. Could you imagine this patch being at fault?

strip: armv7a-unknown-linux-gnueabihf-strip --strip-unneeded -R .comment -R .GCC.command.line -R .note.gnu.gold-version
   lib/libbz2.so.1.0.6
   usr/bin/bzip2recover
   bin/bzip2
   usr/lib/libbz2.a
armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11
armv7a-unknown-linux-gnueabihf-strip: /var/tmp/portage/app-arch/bzip2-1.0.6-r10/image/usr/lib/stImUpsE/bzlib.o: Failed to find link section for section 11

It doesn't seem that likely to me. The error you are seeing looks like a broken object file, the patch here will affect the type of features a target might use during code generation but it shouldn't affect strip.

I suggest dumping the ELF file with readelf or objdump and look at the section output for section 11. Some sections have a special meaning for the sh_link field (http://www.sco.com/developers/gabi/latest/ch4.sheader.html#sh_link) it looks like strip is complaining that either a section is missing a sh_link field that it should have or it has an invalid index.

It will also be worth seeing if the objects in that library have been processed by any other tool such as objcpy to see if it has introduced an error.

lib/Basic/Targets/ARM.cpp

 // FIXME: This should be based on Arch attributes, not CPU names.
 bool ARMTargetInfo::initFeatureMap(
     llvm::StringMap<bool> &Features, DiagnosticsEngine &Diags, StringRef CPU,
     const std::vector<std::string> &FeaturesVec) const {
 
   std::vector<StringRef> TargetFeatures;
   llvm::ARM::ArchKind Arch = llvm::ARM::parseArch(getTriple().getArchName());
 
+  // Map the base architecture to an appropriate target feature, so we don't
+  // rely on the target triple.
+  switch (ArchKind) {
+  case llvm::ARM::ArchKind::INVALID:
+  case llvm::ARM::ArchKind::ARMV2:
+  case llvm::ARM::ArchKind::ARMV2A:
+  case llvm::ARM::ArchKind::ARMV3:
+  case llvm::ARM::ArchKind::ARMV3M:
+  case llvm::ARM::ArchKind::ARMV4:
+    // Base ARM instruction set. (LLVM doesn't support generating code for
+    // anything before v4.)
+    break;
+  case llvm::ARM::ArchKind::ARMV4T:
+    TargetFeatures.push_back("+v4t");
+    break;
+  case llvm::ARM::ArchKind::ARMV5T:
+    TargetFeatures.push_back("+v5t");
+    break;
+  case llvm::ARM::ArchKind::ARMV5TE:
+  case llvm::ARM::ArchKind::IWMMXT:
+  case llvm::ARM::ArchKind::IWMMXT2:
+  case llvm::ARM::ArchKind::XSCALE:
+  case llvm::ARM::ArchKind::ARMV5TEJ:
+    TargetFeatures.push_back("+v5te");
+    break;
+  case llvm::ARM::ArchKind::ARMV6:
+    TargetFeatures.push_back("+v6");
+    break;
+  case llvm::ARM::ArchKind::ARMV6K:
+  case llvm::ARM::ArchKind::ARMV6KZ:
+    TargetFeatures.push_back("+v6k");
+    break;
+  case llvm::ARM::ArchKind::ARMV6T2:
+    TargetFeatures.push_back("+v6t2");
+    break;
+  case llvm::ARM::ArchKind::ARMV6M:
+    TargetFeatures.push_back("+v6m");
+    break;
+  case llvm::ARM::ArchKind::ARMV7A:
+  case llvm::ARM::ArchKind::ARMV7VE:
+  case llvm::ARM::ArchKind::ARMV7R:
+  case llvm::ARM::ArchKind::ARMV7M:
+  case llvm::ARM::ArchKind::ARMV7S:
+  case llvm::ARM::ArchKind::ARMV7EM:
+  case llvm::ARM::ArchKind::ARMV7K:
+    TargetFeatures.push_back("+v7");
+    break;
+  case llvm::ARM::ArchKind::ARMV8A:
+  case llvm::ARM::ArchKind::ARMV8R:
+    TargetFeatures.push_back("+v8");
+    break;
+  case llvm::ARM::ArchKind::ARMV8_1A:
+    TargetFeatures.push_back("+v8.1a");
+    break;
+  case llvm::ARM::ArchKind::ARMV8_2A:
+    TargetFeatures.push_back("+v8.2a");
+    break;
+  case llvm::ARM::ArchKind::ARMV8_3A:
+    TargetFeatures.push_back("+v8.3a");
+    break;
+  case llvm::ARM::ArchKind::ARMV8MBaseline:
+    TargetFeatures.push_back("+v8m");
+    break;
+  case llvm::ARM::ArchKind::ARMV8MMainline:
+    TargetFeatures.push_back("+v8m.main");
+    break;
+  }
+
   // get default FPU features
   unsigned FPUKind = llvm::ARM::getDefaultFPU(CPU, Arch);
   llvm::ARM::getFPUFeatures(FPUKind, TargetFeatures);
 
   // get default Extension features
   unsigned Extensions = llvm::ARM::getDefaultExtensions(CPU, Arch);
   llvm::ARM::getExtensionFeatures(Extensions, TargetFeatures);
 
   for (auto Feature : TargetFeatures)
     if (Feature[0] == '+')

test/CodeGen/arm-long-calls.c

 // RUN: %clang_cc1 -triple thumbv7-apple-ios5  -target-feature +long-calls -emit-llvm -o - %s | FileCheck -check-prefix=LONGCALL %s
 // RUN: %clang_cc1 -triple thumbv7-apple-ios5 -emit-llvm -o - %s | FileCheck -check-prefix=NOLONGCALL %s
 
-// LONGCALL: attributes #0 = { {{.*}} "target-features"="+long-calls,+thumb-mode"
-// NOLONGCALL-NOT: attributes #0 = { {{.*}} "target-features"="+long-calls,+thumb-mode"
+// LONGCALL: attributes #0 = { {{.*}} "target-features"="+long-calls,+thumb-mode,+v7"
+// NOLONGCALL-NOT: attributes #0 = { {{.*}} "target-features"="+long-calls,+thumb-mode,+v7"
 
 int foo1(int a) { return a; }

test/CodeGen/arm-no-movt.c

 // RUN: %clang_cc1 -triple thumbv7-apple-ios5  -target-feature +no-movt -emit-llvm -o - %s | FileCheck -check-prefix=NO-MOVT %s
 // RUN: %clang_cc1 -triple thumbv7-apple-ios5 -emit-llvm -o - %s | FileCheck -check-prefix=MOVT %s
 
-// NO-MOVT: attributes #0 = { {{.*}} "target-features"="+no-movt,+thumb-mode"
-// MOVT-NOT: attributes #0 = { {{.*}} "target-features"="+no-movt,+thumb-mode"
+// NO-MOVT: attributes #0 = { {{.*}} "target-features"="+no-movt,+thumb-mode,+v7"
+// MOVT-NOT: attributes #0 = { {{.*}} "target-features"="+no-movt,+thumb-mode,+v7"
 
 int foo1(int a) { return a; }

test/CodeGen/arm-target-features.c

 // REQUIRES: arm-registered-target
 
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabihf -target-cpu cortex-a8 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP3
-// CHECK-VFP3: "target-features"="+dsp,+neon,+thumb-mode
+// CHECK-VFP3: "target-features"="+dsp,+neon,+thumb-mode,+v7,+vfp3"
 
 
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabihf -target-cpu cortex-a5 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4
-// CHECK-VFP4: "target-features"="+dsp,+neon,+thumb-mode,+vfp4"
+// CHECK-VFP4: "target-features"="+dsp,+neon,+thumb-mode,+v7,+vfp4"
 
 
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabihf -target-cpu cortex-a7 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabi -target-cpu cortex-a12 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV
 // RUN: %clang_cc1 -triple thumbv7s-linux-gnueabi -target-cpu swift -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabihf -target-cpu krait -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV
-// CHECK-VFP4-DIV: "target-features"="+dsp,+hwdiv,+hwdiv-arm,+neon,+thumb-mode,+vfp4"
+// CHECK-VFP4-DIV: "target-features"="+dsp,+hwdiv,+hwdiv-arm,+neon,+thumb-mode,+v7,+vfp4"
 
 // RUN: %clang_cc1 -triple armv7-linux-gnueabihf -target-cpu cortex-a15 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV-ARM
 // RUN: %clang_cc1 -triple armv7-linux-gnueabihf -target-cpu cortex-a17 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-DIV-ARM
-// CHECK-VFP4-DIV-ARM: "target-features"="+dsp,+hwdiv,+hwdiv-arm,+neon,+vfp4,-thumb-mode"
+// CHECK-VFP4-DIV-ARM: "target-features"="+dsp,+hwdiv,+hwdiv-arm,+neon,+v7,+vfp4,-thumb-mode"
 
 // RUN: %clang_cc1 -triple thumbv7s-apple-ios7.0 -target-cpu cyclone -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
 // RUN: %clang_cc1 -triple thumbv8-linux-gnueabihf -target-cpu cortex-a32 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
 // RUN: %clang_cc1 -triple thumbv8-linux-gnueabihf -target-cpu cortex-a35 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
 // RUN: %clang_cc1 -triple thumbv8-linux-gnueabihf -target-cpu cortex-a57 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
 // RUN: %clang_cc1 -triple thumbv8-linux-gnueabihf -target-cpu cortex-a72 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
 // RUN: %clang_cc1 -triple thumbv8-linux-gnueabihf -target-cpu cortex-a73 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
 // RUN: %clang_cc1 -triple thumbv8-linux-gnueabihf -target-cpu exynos-m1 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
 // RUN: %clang_cc1 -triple thumbv8-linux-gnueabihf -target-cpu exynos-m2 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
 // RUN: %clang_cc1 -triple thumbv8-linux-gnueabihf -target-cpu exynos-m3 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8
-// CHECK-BASIC-V8: "target-features"="+crc,+crypto,+dsp,+fp-armv8,+hwdiv,+hwdiv-arm,+neon,+thumb-mode"
+// CHECK-BASIC-V8: "target-features"="+crc,+crypto,+dsp,+fp-armv8,+hwdiv,+hwdiv-arm,+neon,+thumb-mode,+v8"
 
 // RUN: %clang_cc1 -triple armv8-linux-gnueabi -target-cpu cortex-a53 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-BASIC-V8-ARM
-// CHECK-BASIC-V8-ARM: "target-features"="+crc,+crypto,+dsp,+fp-armv8,+hwdiv,+hwdiv-arm,+neon,-thumb-mode"
+// CHECK-BASIC-V8-ARM: "target-features"="+crc,+crypto,+dsp,+fp-armv8,+hwdiv,+hwdiv-arm,+neon,+v8,-thumb-mode"
 
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabi -target-cpu cortex-r5 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP3-D16-DIV
-// CHECK-VFP3-D16-DIV: "target-features"="+d16,+dsp,+hwdiv,+hwdiv-arm,+thumb-mode,+vfp3"
+// CHECK-VFP3-D16-DIV: "target-features"="+d16,+dsp,+hwdiv,+hwdiv-arm,+thumb-mode,+v7,+vfp3"
 
 
 // RUN: %clang_cc1 -triple armv7-linux-gnueabi -target-cpu cortex-r4f -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP3-D16-THUMB-DIV
-// CHECK-VFP3-D16-THUMB-DIV: "target-features"="+d16,+dsp,+hwdiv,+vfp3,-thumb-mode"
+// CHECK-VFP3-D16-THUMB-DIV: "target-features"="+d16,+dsp,+hwdiv,+v7,+vfp3,-thumb-mode"
 
 
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabi -target-cpu cortex-r7 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP3-D16-FP16-DIV
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabi -target-cpu cortex-r8 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP3-D16-FP16-DIV
-// CHECK-VFP3-D16-FP16-DIV: "target-features"="+d16,+dsp,+fp16,+hwdiv,+hwdiv-arm,+thumb-mode,+vfp3"
+// CHECK-VFP3-D16-FP16-DIV: "target-features"="+d16,+dsp,+fp16,+hwdiv,+hwdiv-arm,+thumb-mode,+v7,+vfp3"
 
 
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabi -target-cpu cortex-m4 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP4-D16-SP-THUMB-DIV
-// CHECK-VFP4-D16-SP-THUMB-DIV: "target-features"="+d16,+dsp,+fp-only-sp,+hwdiv,+thumb-mode,+vfp4"
+// CHECK-VFP4-D16-SP-THUMB-DIV: "target-features"="+d16,+dsp,+fp-only-sp,+hwdiv,+thumb-mode,+v7,+vfp4"
 
 
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabi -target-cpu cortex-m7 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-VFP5-D16-THUMB-DIV
-// CHECK-VFP5-D16-THUMB-DIV: "target-features"="+d16,+dsp,+fp-armv8,+hwdiv,+thumb-mode"
+// CHECK-VFP5-D16-THUMB-DIV: "target-features"="+d16,+dsp,+fp-armv8,+hwdiv,+thumb-mode,+v7"
 
 
 // RUN: %clang_cc1 -triple armv7-linux-gnueabi -target-cpu cortex-r4 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-THUMB-DIV
-// CHECK-THUMB-DIV: "target-features"="+dsp,+hwdiv,-thumb-mode"
+// CHECK-THUMB-DIV: "target-features"="+dsp,+hwdiv,+v7,-thumb-mode"
 
 // RUN: %clang_cc1 -triple thumbv7-linux-gnueabi -target-cpu cortex-m3 -emit-llvm -o - %s | FileCheck %s --check-prefix=CHECK-THUMB-DIV-M3
-// CHECK-THUMB-DIV-M3: "target-features"="+hwdiv,+thumb-mode"
+// CHECK-THUMB-DIV-M3: "target-features"="+hwdiv,+thumb-mode,+v7"
 
 
 void foo() {}

test/CodeGen/arm-thumb-mode-target-feature.c

 void t2() {}
 
  __attribute__((target("thumb-mode")))
 void t3() {}
 
 // THUMB: void @t1() [[ThumbAttr:#[0-7]]]
 // THUMB: void @t2() [[NoThumbAttr:#[0-7]]]
 // THUMB: void @t3() [[ThumbAttr:#[0-7]]]
-// THUMB: attributes [[ThumbAttr]] = { {{.*}} "target-features"="+thumb-mode"
-// THUMB: attributes [[NoThumbAttr]] = { {{.*}} "target-features"="-thumb-mode"
+// THUMB: attributes [[ThumbAttr]] = { {{.*}} "target-features"="+thumb-mode,+v7"
+// THUMB: attributes [[NoThumbAttr]] = { {{.*}} "target-features"="+v7,-thumb-mode"
 //
 // THUMB-CLANG: void @t1() [[ThumbAttr:#[0-7]]]
 // THUMB-CLANG: void @t2() [[NoThumbAttr:#[0-7]]]
 // THUMB-CLANG: void @t3() [[ThumbAttr:#[0-7]]]
 // THUMB-CLANG: attributes [[ThumbAttr]] = { {{.*}} "target-features"="{{.*}}+thumb-mode
 // THUMB-CLANG: attributes [[NoThumbAttr]] = { {{.*}} "target-features"="{{.*}}-thumb-mode
 
 // ARM: void @t1() [[NoThumbAtr:#[0-7]]]
 // ARM: void @t2() [[NoThumbAttr:#[0-7]]]
 // ARM: void @t3() [[ThumbAttr:#[0-7]]]
-// ARM: attributes [[NoThumbAttr]] = { {{.*}} "target-features"="-thumb-mode"
-// ARM: attributes [[ThumbAttr]] = { {{.*}} "target-features"="+thumb-mode"
+// ARM: attributes [[NoThumbAttr]] = { {{.*}} "target-features"="+v7,-thumb-mode"
+// ARM: attributes [[ThumbAttr]] = { {{.*}} "target-features"="+thumb-mode,+v7"