Cull non-standard variants of ARM architectures (NFC)

[tyomitch]

Summary

This patch changes ARMV5, ARMV5E, ARMV6SM, ARMV6HL, ARMV7, ARMV7L,
ARMV7HL, ARMV7EM to be treated as aliases for the corresponding
standard architectures, instead of as actual architectures.

[rengolin]

What's the motivation behind this?

These names are used for -march and triples, which are still in use by many build systems out there (and why they were introduced in the first place).

I don't see why we'd want to remove them, other than break legacy systems.

[tyomitch]

In D14577#287416, @rengolin wrote:

What's the motivation behind this?

These names are used for -march and triples, which are still in use by many build systems out there (and why they were introduced in the first place).

I don't see why we'd want to remove them, other than break legacy systems.

The motivation is to still accept them in -march and triples (as aliases), while cleaning them out of the rest of the source code.

The user interface stays unaffected.

[rengolin]

In D14577#287449, @tyomitch wrote:

The motivation is to still accept them in -march and triples (as aliases), while cleaning them out of the rest of the source code.

The user interface stays unaffected.

No, it doesn't. You're removing more than you're putting back.

Even if all current tests continue to pass, it doesn't mean that the change has no functional changes, just that the tests weren't picking all the changes up.

For example, you're changing v7hl into v7-a, which in itself is "correct" but can have unpredictable side effects on corner cases. Those functions are called all over the place for different reasons, some of them only relevant to the internal logic of the function, like depending on the sub-arch, choose different options. These places are used on Linux, bare-metal, Darwin and Windows, with different results for each one.

The original state was that every place had its own parsing, which was really bad. Now, all of them are using the same parsing, but that raises a lot of bloat, redundancies and non-canonical naming, which is still bad, but not as much.

Removing a lot of legacy like this is bound to break a lot of things we aren't testing for, for no other reason than "just a clean up".

Normally, I'd be supportive of clean ups, but this part is a minefield, and we have been bitten before, every time we assume something was not being used any more. I don't think it's a good idea.

[tyomitch]

In D14577#287461, @rengolin wrote:

In D14577#287449, @tyomitch wrote:

The motivation is to still accept them in -march and triples (as aliases), while cleaning them out of the rest of the source code.

The user interface stays unaffected.

No, it doesn't. You're removing more than you're putting back.

Even if all current tests continue to pass, it doesn't mean that the change has no functional changes, just that the tests weren't picking all the changes up.

For example, you're changing v7hl into v7-a, which in itself is "correct" but can have unpredictable side effects on corner cases. Those functions are called all over the place for different reasons, some of them only relevant to the internal logic of the function, like depending on the sub-arch, choose different options. These places are used on Linux, bare-metal, Darwin and Windows, with different results for each one.

I don't see what you mean.
I'm removing all uses of AK_ARMV7HL as part of the patch; there were only two, and in both cases it was treated identically to AK_ARMV7A.
How can, therefore, my change introduce any change in functionality?

[rengolin]

So, you removed the CPU names, but not the arch names. Try removing the AK_* types from the CPUs that you remove and see if anything breaks...

include/llvm/Support/ARMTargetParser.def

183

Did you look through all the options that use AK_ARMV5TE or AK_XSCALE? Are they always being treated in the same way every time?

233

This CPU name was an old one that was added for some reason I don't remember... :( some GCC compatibility, I guess. IIRC, this was well before we created the TargetParser.

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

704

I guess there isn't a v5 without Thumb...

[tyomitch]

In D14577#287543, @rengolin wrote:

So, you removed the CPU names, but not the arch names. Try removing the AK_* types from the CPUs that you remove and see if anything breaks...

Yes I did delete them (lines 77-111 in ARMTargetParser.def)

include/llvm/Support/ARMTargetParser.def

183

I don't touch either AK_ARMV5TE or AK_XSCALE.

These two definitions are no-ops: using "iwmmxt" or "xscale" as the CPU name is interpreted as AK_IWMMXT or AK_XSCALE, correspondingly, due to the definitions further down.

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

704

That's right: such an architecture was once defined, but deprecated straight away, and never implemented by any vendor.

https://www.scss.tcd.ie/~waldroj/3d1/arm_arm.pdf (dated 2005) says:

The valid architecture variants are as follows (variant in brackets for legacy reasons only):

• ARMv4, ARMv4T, ARMv5T, (ARMv5TExP), ARMv5TE, ARMv5TEJ, and ARMv6

The following architecture variants are now OBSOLETE:

• ARMv1, ARMv2, ARMv2a, ARMv3, ARMv3G, ARMv3M, ARMv4xM, ARMv4TxM, ARMv5, ARMv5xM, and ARMv5TxM

[rengolin]

Sigh, the target parser used to make sense, now I don't remember half of the decisions, and they're all washed and moved everywhere.

I guess the best way forward is, indeed, to simplify and wait for back slash.

At least if there is a legacy usage we can add them back and add tests to make sure it's explicit.

Go ahead and commit and let's see what gives.

[tyomitch]

Thanks! What about the other one, ARMv6KZ-related? (D14568)

include/llvm/Support/ARMTargetParser.def

 ARM_ARCH("armv6k", AK_ARMV6K, "6K", "v6k", ARMBuildAttrs::CPUArch::v6K,
           AEK_DSP)
 ARM_ARCH("armv6t2", AK_ARMV6T2, "6T2", "v6t2", ARMBuildAttrs::CPUArch::v6T2,
           AEK_DSP)
 ARM_ARCH("armv6kz", AK_ARMV6KZ, "6KZ", "v6kz", ARMBuildAttrs::CPUArch::v6KZ,
           (AEK_SEC | AEK_DSP))
 ARM_ARCH("armv6-m", AK_ARMV6M, "6-M", "v6m", ARMBuildAttrs::CPUArch::v6_M,
           AEK_NONE)
-ARM_ARCH("armv6s-m", AK_ARMV6SM, "6S-M", "v6sm", ARMBuildAttrs::CPUArch::v6S_M,
-          AEK_NONE)
 ARM_ARCH("armv7-a", AK_ARMV7A, "7-A", "v7", ARMBuildAttrs::CPUArch::v7,
           AEK_DSP)
 ARM_ARCH("armv7-r", AK_ARMV7R, "7-R", "v7r", ARMBuildAttrs::CPUArch::v7,
           (AEK_HWDIV | AEK_DSP))
 ARM_ARCH("armv7-m", AK_ARMV7M, "7-M", "v7m", ARMBuildAttrs::CPUArch::v7,
           AEK_HWDIV)
 ARM_ARCH("armv7e-m", AK_ARMV7EM, "7E-M", "v7em", ARMBuildAttrs::CPUArch::v7E_M,
           (AEK_HWDIV | AEK_DSP))
 ARM_ARCH("armv8-a", AK_ARMV8A, "8-A", "v8", ARMBuildAttrs::CPUArch::v8,
          (AEK_SEC | AEK_MP | AEK_VIRT | AEK_HWDIVARM | AEK_HWDIV | AEK_DSP))
 ARM_ARCH("armv8.1-a", AK_ARMV8_1A, "8.1-A", "v8.1a", ARMBuildAttrs::CPUArch::v8,
          (AEK_SEC | AEK_MP | AEK_VIRT | AEK_HWDIVARM | AEK_HWDIV | AEK_DSP))
 // Non-standard Arch names.
 ARM_ARCH("iwmmxt", AK_IWMMXT, "iwmmxt", "", ARMBuildAttrs::CPUArch::v5TE,
           AEK_NONE)
 ARM_ARCH("iwmmxt2", AK_IWMMXT2, "iwmmxt2", "", ARMBuildAttrs::CPUArch::v5TE,
           AEK_NONE)
 ARM_ARCH("xscale", AK_XSCALE, "xscale", "", ARMBuildAttrs::CPUArch::v5TE,
           AEK_NONE)
-ARM_ARCH("armv5", AK_ARMV5, "5T", "v5", ARMBuildAttrs::CPUArch::v5T,
-          AEK_NONE)
-ARM_ARCH("armv5e", AK_ARMV5E, "5TE", "v5e", ARMBuildAttrs::CPUArch::v5TE,
-          AEK_DSP)
 ARM_ARCH("armv6j", AK_ARMV6J, "6J", "v6", ARMBuildAttrs::CPUArch::v6,
           AEK_DSP)
-ARM_ARCH("armv6hl", AK_ARMV6HL, "6-M", "v6hl", ARMBuildAttrs::CPUArch::v6_M,
-          AEK_NONE)
-ARM_ARCH("armv7", AK_ARMV7, "7", "v7", ARMBuildAttrs::CPUArch::v7,
-          AEK_NONE)
-ARM_ARCH("armv7l", AK_ARMV7L, "7-L", "v7l", ARMBuildAttrs::CPUArch::v7,
-          AEK_DSP)
-ARM_ARCH("armv7hl", AK_ARMV7HL, "7-L", "v7hl", ARMBuildAttrs::CPUArch::v7,
-          AEK_DSP)
 ARM_ARCH("armv7s", AK_ARMV7S, "7-S", "v7s", ARMBuildAttrs::CPUArch::v7,
           AEK_DSP)
 ARM_ARCH("armv7k", AK_ARMV7K, "7-K", "v7k", ARMBuildAttrs::CPUArch::v7,
           AEK_DSP)
 #undef ARM_ARCH
 
 #ifndef ARM_ARCH_EXT_NAME
 #define ARM_ARCH_EXT_NAME(NAME, ID)

 ARM_CPU_NAME("arm1020t", AK_ARMV5T, FK_NONE, false, AEK_NONE)
 ARM_CPU_NAME("arm9e", AK_ARMV5TE, FK_NONE, false, AEK_NONE)
 ARM_CPU_NAME("arm946e-s", AK_ARMV5TE, FK_NONE, false, AEK_NONE)
 ARM_CPU_NAME("arm966e-s", AK_ARMV5TE, FK_NONE, false, AEK_NONE)
 ARM_CPU_NAME("arm968e-s", AK_ARMV5TE, FK_NONE, false, AEK_NONE)
 ARM_CPU_NAME("arm10e", AK_ARMV5TE, FK_NONE, false, AEK_NONE)
 ARM_CPU_NAME("arm1020e", AK_ARMV5TE, FK_NONE, false, AEK_NONE)
 ARM_CPU_NAME("arm1022e", AK_ARMV5TE, FK_NONE, true, AEK_NONE)
-ARM_CPU_NAME("iwmmxt", AK_ARMV5TE, FK_NONE, false, AEK_NONE)
-ARM_CPU_NAME("xscale", AK_ARMV5TE, FK_NONE, false, AEK_NONE)
 ARM_CPU_NAME("arm926ej-s", AK_ARMV5TEJ, FK_NONE, true, AEK_NONE)
 ARM_CPU_NAME("arm1136jf-s", AK_ARMV6, FK_VFPV2, true, AEK_NONE)
 ARM_CPU_NAME("arm1176j-s", AK_ARMV6K, FK_NONE, true, AEK_NONE)
 ARM_CPU_NAME("arm1176jz-s", AK_ARMV6KZ, FK_NONE, false, AEK_NONE)
 ARM_CPU_NAME("mpcore", AK_ARMV6K, FK_VFPV2, false, AEK_NONE)
 ARM_CPU_NAME("mpcorenovfp", AK_ARMV6K, FK_NONE, false, AEK_NONE)
 ARM_CPU_NAME("arm1176jzf-s", AK_ARMV6KZ, FK_VFPV2, true, AEK_NONE)
 ARM_CPU_NAME("arm1156t2-s", AK_ARMV6T2, FK_NONE, true, AEK_NONE)

 ARM_CPU_NAME("cortex-a53", AK_ARMV8A, FK_CRYPTO_NEON_FP_ARMV8, true, AEK_CRC)
 ARM_CPU_NAME("cortex-a57", AK_ARMV8A, FK_CRYPTO_NEON_FP_ARMV8, false, AEK_CRC)
 ARM_CPU_NAME("cortex-a72", AK_ARMV8A, FK_CRYPTO_NEON_FP_ARMV8, false, AEK_CRC)
 ARM_CPU_NAME("cyclone", AK_ARMV8A, FK_CRYPTO_NEON_FP_ARMV8, false, AEK_CRC)
 ARM_CPU_NAME("generic", AK_ARMV8_1A, FK_NEON_FP_ARMV8, true, AEK_NONE)
 // Non-standard Arch names.
 ARM_CPU_NAME("iwmmxt", AK_IWMMXT, FK_NONE, true, AEK_NONE)
 ARM_CPU_NAME("xscale", AK_XSCALE, FK_NONE, true, AEK_NONE)
-ARM_CPU_NAME("arm10tdmi", AK_ARMV5, FK_NONE, true, AEK_NONE)
-ARM_CPU_NAME("arm1022e", AK_ARMV5E, FK_NONE, true, AEK_NONE)
 ARM_CPU_NAME("arm1136j-s", AK_ARMV6J, FK_NONE, true, AEK_NONE)
 ARM_CPU_NAME("arm1136jz-s", AK_ARMV6J, FK_NONE, false, AEK_NONE)
-ARM_CPU_NAME("cortex-m0", AK_ARMV6SM, FK_NONE, true, AEK_NONE)
-ARM_CPU_NAME("arm1176jzf-s", AK_ARMV6HL, FK_VFPV2, true, AEK_NONE)
-ARM_CPU_NAME("cortex-a8", AK_ARMV7, FK_NEON, true, AEK_SEC)
-ARM_CPU_NAME("cortex-a8", AK_ARMV7L, FK_NEON, true, AEK_SEC)
-ARM_CPU_NAME("cortex-a8", AK_ARMV7HL, FK_NEON, true, AEK_SEC)
-ARM_CPU_NAME("cortex-m4", AK_ARMV7EM, FK_NONE, true, AEK_NONE)
 ARM_CPU_NAME("swift", AK_ARMV7S, FK_NEON_VFPV4, true,
              (AEK_HWDIVARM | AEK_HWDIV))
 ARM_CPU_NAME("cortex-a7", AK_ARMV7K, FK_NONE, true, AEK_HWDIVARM | AEK_HWDIV)
 // Invalid CPU
 ARM_CPU_NAME("invalid", AK_INVALID, FK_INVALID, true, AEK_INVALID)
 #undef ARM_CPU_NAME

lib/Support/TargetParser.cpp

return StringSwitch<StringRef>(FPU)
       .Cases("fp5-dp-d16", "fpv5-dp-d16", "fpv5-d16")
       // FIXME: Clang uses it, but it's bogus, since neon defaults to vfpv3.
       .Case("neon-vfpv3", "neon")
       .Default(FPU);
 }
 
 static StringRef getArchSynonym(StringRef Arch) {
   return StringSwitch<StringRef>(Arch)
+      .Case("v5", "v5t")
+      .Case("v5e", "v5te")
+      .Case("v6hl", "v6k")
+      .Cases("v6m", "v6sm", "v6s-m", "v6-m")
       .Cases("v6z", "v6zk", "v6kz")
-      .Case("v6sm", "v6s-m")
-      .Case("v6m", "v6-m")
-      .Case("v7a", "v7-a")
+      .Cases("v7", "v7a", "v7hl", "v7l", "v7-a")
       .Case("v7r", "v7-r")
       .Case("v7m", "v7-m")
       .Case("v7em", "v7e-m")
       .Cases("v8", "v8a", "aarch64", "arm64", "v8-a")
       .Case("v8.1a", "v8.1-a")
       .Default(Arch);
 }
 

 }
 
 // Profile A/R/M
 unsigned llvm::ARM::parseArchProfile(StringRef Arch) {
   Arch = getCanonicalArchName(Arch);
   switch (parseArch(Arch)) {
   case ARM::AK_ARMV6M:
   case ARM::AK_ARMV7M:
-  case ARM::AK_ARMV6SM:
   case ARM::AK_ARMV7EM:
     return ARM::PK_M;
   case ARM::AK_ARMV7R:
     return ARM::PK_R;
-  case ARM::AK_ARMV7:
   case ARM::AK_ARMV7A:
   case ARM::AK_ARMV7K:
-  case ARM::AK_ARMV7L:
   case ARM::AK_ARMV8A:
   case ARM::AK_ARMV8_1A:
     return ARM::PK_A;
   }
   return ARM::PK_INVALID;
 }
 
 // Version number (ex. v7 = 7).
 unsigned llvm::ARM::parseArchVersion(StringRef Arch) {
   Arch = getCanonicalArchName(Arch);
   switch (parseArch(Arch)) {
   case ARM::AK_ARMV2:
   case ARM::AK_ARMV2A:
     return 2;
   case ARM::AK_ARMV3:
   case ARM::AK_ARMV3M:
     return 3;
   case ARM::AK_ARMV4:
   case ARM::AK_ARMV4T:
     return 4;
-  case ARM::AK_ARMV5:
   case ARM::AK_ARMV5T:
   case ARM::AK_ARMV5TE:
   case ARM::AK_IWMMXT:
   case ARM::AK_IWMMXT2:
   case ARM::AK_XSCALE:
-  case ARM::AK_ARMV5E:
   case ARM::AK_ARMV5TEJ:
     return 5;
   case ARM::AK_ARMV6:
   case ARM::AK_ARMV6J:
   case ARM::AK_ARMV6K:
   case ARM::AK_ARMV6T2:
   case ARM::AK_ARMV6KZ:
   case ARM::AK_ARMV6M:
-  case ARM::AK_ARMV6SM:
-  case ARM::AK_ARMV6HL:
     return 6;
-  case ARM::AK_ARMV7:
   case ARM::AK_ARMV7A:
   case ARM::AK_ARMV7R:
   case ARM::AK_ARMV7M:
-  case ARM::AK_ARMV7L:
-  case ARM::AK_ARMV7HL:
   case ARM::AK_ARMV7S:
   case ARM::AK_ARMV7EM:
   case ARM::AK_ARMV7K:
     return 7;
   case ARM::AK_ARMV8A:
   case ARM::AK_ARMV8_1A:
     return 8;
   }
   return 0;
 }

lib/Support/Triple.cpp

return StringSwitch<Triple::SubArchType>(SubArchName)
       .Default(Triple::NoSubArch);
 
   // ARM sub arch.
   switch(ARM::parseArch(ARMSubArch)) {
   case ARM::AK_ARMV4:
     return Triple::NoSubArch;
   case ARM::AK_ARMV4T:
     return Triple::ARMSubArch_v4t;
-  case ARM::AK_ARMV5:
   case ARM::AK_ARMV5T:
-  case ARM::AK_ARMV5E:
     return Triple::ARMSubArch_v5;
   case ARM::AK_ARMV5TE:
   case ARM::AK_IWMMXT:
   case ARM::AK_IWMMXT2:
   case ARM::AK_XSCALE:
   case ARM::AK_ARMV5TEJ:
     return Triple::ARMSubArch_v5te;
   case ARM::AK_ARMV6:
   case ARM::AK_ARMV6J:
     return Triple::ARMSubArch_v6;
   case ARM::AK_ARMV6K:
   case ARM::AK_ARMV6KZ:
-  case ARM::AK_ARMV6HL:
     return Triple::ARMSubArch_v6k;
   case ARM::AK_ARMV6T2:
     return Triple::ARMSubArch_v6t2;
   case ARM::AK_ARMV6M:
-  case ARM::AK_ARMV6SM:
     return Triple::ARMSubArch_v6m;
-  case ARM::AK_ARMV7:
   case ARM::AK_ARMV7A:
   case ARM::AK_ARMV7R:
-  case ARM::AK_ARMV7L:
-  case ARM::AK_ARMV7HL:
     return Triple::ARMSubArch_v7;
   case ARM::AK_ARMV7K:
     return Triple::ARMSubArch_v7k;
   case ARM::AK_ARMV7M:
     return Triple::ARMSubArch_v7m;
   case ARM::AK_ARMV7S:
     return Triple::ARMSubArch_v7s;
   case ARM::AK_ARMV7EM:

lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp

 
 static MachO::CPUSubTypeARM getMachOSubTypeFromArch(StringRef Arch) {
   unsigned AK = ARM::parseArch(Arch);
   switch (AK) {
   default:
     return MachO::CPU_SUBTYPE_ARM_V7;
   case ARM::AK_ARMV4T:
     return MachO::CPU_SUBTYPE_ARM_V4T;
-  case ARM::AK_ARMV6:
-  case ARM::AK_ARMV6K:
-    return MachO::CPU_SUBTYPE_ARM_V6;
-  case ARM::AK_ARMV5:
-    return MachO::CPU_SUBTYPE_ARM_V5;
   case ARM::AK_ARMV5T:
-  case ARM::AK_ARMV5E:
   case ARM::AK_ARMV5TE:
   case ARM::AK_ARMV5TEJ:
-    return MachO::CPU_SUBTYPE_ARM_V5TEJ;
-  case ARM::AK_ARMV7:
+    return MachO::CPU_SUBTYPE_ARM_V5;
+  case ARM::AK_ARMV6:
+  case ARM::AK_ARMV6K:
+    return MachO::CPU_SUBTYPE_ARM_V6;
+  case ARM::AK_ARMV7A:
     return MachO::CPU_SUBTYPE_ARM_V7;
   case ARM::AK_ARMV7S:
     return MachO::CPU_SUBTYPE_ARM_V7S;
   case ARM::AK_ARMV7K:
     return MachO::CPU_SUBTYPE_ARM_V7K;
   case ARM::AK_ARMV6M:
-  case ARM::AK_ARMV6SM:
     return MachO::CPU_SUBTYPE_ARM_V6M;
   case ARM::AK_ARMV7M:
     return MachO::CPU_SUBTYPE_ARM_V7M;
   case ARM::AK_ARMV7EM:
     return MachO::CPU_SUBTYPE_ARM_V7EM;
   }
 }
 

lib/Target/ARM/MCTargetDesc/ARMELFStreamer.cpp

setAttributeItem(CPU_arch,
                      false);
 
   switch (Arch) {
   case ARM::AK_ARMV2:
   case ARM::AK_ARMV2A:
   case ARM::AK_ARMV3:
   case ARM::AK_ARMV3M:
   case ARM::AK_ARMV4:
-  case ARM::AK_ARMV5:
     setAttributeItem(ARM_ISA_use, Allowed, false);
     break;
 
   case ARM::AK_ARMV4T:
   case ARM::AK_ARMV5T:
   case ARM::AK_ARMV5TE:
   case ARM::AK_ARMV6:
   case ARM::AK_ARMV6J:
case ARM::AK_ARMV6KZ:
     setAttributeItem(THUMB_ISA_use, Allowed, false);
     setAttributeItem(Virtualization_use, AllowTZ, false);
     break;
 
   case ARM::AK_ARMV6M:
     setAttributeItem(THUMB_ISA_use, Allowed, false);
     break;
 
-  case ARM::AK_ARMV7:
-    setAttributeItem(THUMB_ISA_use, AllowThumb32, false);
-    break;
-
   case ARM::AK_ARMV7A:
     setAttributeItem(CPU_arch_profile, ApplicationProfile, false);
     setAttributeItem(ARM_ISA_use, Allowed, false);
     setAttributeItem(THUMB_ISA_use, AllowThumb32, false);
     break;
 
   case ARM::AK_ARMV7R:
     setAttributeItem(CPU_arch_profile, RealTimeProfile, false);