Refactor ARM subarchitecture parsing.

[enefaim]

Summary

According to a FIXME in ARMMCTargetDesc.cpp the ARM version parsing should be in the Triple helper class.

[rengolin]

Hi Gabor,

Thanks for working on this, looks a lot better! I think we have tests for all of them, and I assume they're all passing.

With the changes below, LGTM.

cheers,
--renato

include/llvm/ADT/Triple.h

81

I'd number them in sequential order, so you can do tricks like Arch > v5.

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

163

you could add the NoSubArch here, as a default.

[t.p.northover]

include/llvm/ADT/Triple.h

81

I actually quite like the original: comparing architectures like that on ARM is extremely suspect since they form a partial order (Is v6m > v4t?).

Sorting in some kind of reverse (as here) reduces the temptation to do that since even the obvious comparisons will be "wrong".

84

Triple is generic code. At the very least we should prefix these with "ARMSubArch" or something to avoid polluting other targets.

[rengolin]

include/llvm/ADT/Triple.h

81

Using reverse order allows you to compare, but in the wrong order, which is even worse... :)

I agree many comparisons will be "wrong", but some are still "ok", and having them in order would make it less weird.

But this is just a weak opinion, I'm fine either way.

84

Good point!

[enefaim]

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

163

I tried to add it, but it made 12 tests fail:
Failing Tests (12):

LLVM :: CodeGen/ARM/2010-03-18-ldm-rtrn.ll
LLVM :: CodeGen/ARM/2013-04-05-Small-ByVal-Structs-PR15293.ll
LLVM :: CodeGen/ARM/2014-02-21-byval-reg-split-alignment.ll
LLVM :: CodeGen/ARM/arguments.ll
LLVM :: CodeGen/ARM/armv4.ll
LLVM :: CodeGen/ARM/call_nolink.ll
LLVM :: CodeGen/ARM/debug-frame.ll
LLVM :: CodeGen/ARM/ehabi.ll
LLVM :: CodeGen/ARM/inlineasm-ldr-pseudo.ll
LLVM :: CodeGen/ARM/integer_insertelement.ll
LLVM :: CodeGen/ARM/jump_tables.ll
LLVM :: MC/ARM/arm-thumb-cpus.s

[rengolin]

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

163

Interesting... Maybe the default sub-arch should be v4 not v4t.

[enefaim]

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

163

Yes, that works. I'm going to add 'v4' as default, and add the ARMSubArch prefixes that Tim suggested.

[rengolin]

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

163

Sounds good, thanks!

[enefaim]

Added 'ARMSubArch' prefix for the SubArchType enum.
Added 'v4' as default ARMArchFeature in the ParseArmTriple function.

[rengolin]

Hi Gabor,

I leave the decision of adding a specific v4 to you, I don't mind much. Otherwise, LGTM.

Thanks!
--renato

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

166

I'm thinking if we need an ARMSubArch_v4 as well as the NoSubArch...

While it feels wrong to not have it, I don't think it matters much...

[enefaim]

Added ARMSubArch_v4 to the SubArch enum and the ParseArmTriple function.

Renato, could you land it for me? I don't have commit rights.

Thanks,
Gabor

[rengolin]

Committed in r212479.

cheers,
--renato

include/llvm/ADT/Triple.h

     xcore,      // XCore: xcore
     nvptx,      // NVPTX: 32-bit
     nvptx64,    // NVPTX: 64-bit
     le32,       // le32: generic little-endian 32-bit CPU (PNaCl / Emscripten)
     amdil,      // amdil: amd IL
     spir,       // SPIR: standard portable IR for OpenCL 32-bit version
     spir64      // SPIR: standard portable IR for OpenCL 64-bit version
   };
+  enum SubArchType {
+    NoSubArch,
+
+    ARMSubArch_v8,
+    ARMSubArch_v7,
+    ARMSubArch_v7em,
+    ARMSubArch_v7m,
+    ARMSubArch_v7s,
+    ARMSubArch_v6,
+    ARMSubArch_v6m,
+    ARMSubArch_v6t2,
+    ARMSubArch_v5,
+    ARMSubArch_v5te,
+    ARMSubArch_v4t,
+    ARMSubArch_v4
+  };
   enum VendorType {
     UnknownVendor,
 
     Apple,
     PC,
     SCEI,
     BGP,
     BGQ,

   };
 
 private:
   std::string Data;
 
   /// The parsed arch type.
   ArchType Arch;
 
+  /// The parsed subarchitecture type.
+  SubArchType SubArch;
+
   /// The parsed vendor type.
   VendorType Vendor;
 
   /// The parsed OS type.
   OSType OS;
 
   /// The parsed Environment type.
   EnvironmentType Environment;

 
   /// @}
   /// @name Typed Component Access
   /// @{
 
   /// getArch - Get the parsed architecture type of this triple.
   ArchType getArch() const { return Arch; }
 
+  /// getSubArch - get the parsed subarchitecture type for this triple.
+  SubArchType getSubArch() const { return SubArch; }
+
   /// getVendor - Get the parsed vendor type of this triple.
   VendorType getVendor() const { return Vendor; }
 
   /// getOS - Get the parsed operating system type of this triple.
   OSType getOS() const { return OS; }
 
   /// hasEnvironment - Does this triple have the optional environment
   /// (fourth) component?

lib/Support/Triple.cpp

 static Triple::ObjectFormatType parseFormat(StringRef EnvironmentName) {
   return StringSwitch<Triple::ObjectFormatType>(EnvironmentName)
     .EndsWith("coff", Triple::COFF)
     .EndsWith("elf", Triple::ELF)
     .EndsWith("macho", Triple::MachO)
     .Default(Triple::UnknownObjectFormat);
 }
 
+static Triple::SubArchType parseSubArch(StringRef SubArchName) {
+  return StringSwitch<Triple::SubArchType>(SubArchName)
+    .EndsWith("v8", Triple::ARMSubArch_v8)
+    .EndsWith("v8a", Triple::ARMSubArch_v8)
+    .EndsWith("v7", Triple::ARMSubArch_v7)
+    .EndsWith("v7a", Triple::ARMSubArch_v7)
+    .EndsWith("v7em", Triple::ARMSubArch_v7em)
+    .EndsWith("v7m", Triple::ARMSubArch_v7m)
+    .EndsWith("v7s", Triple::ARMSubArch_v7s)
+    .EndsWith("v6", Triple::ARMSubArch_v6)
+    .EndsWith("v6m", Triple::ARMSubArch_v6m)
+    .EndsWith("v6t2", Triple::ARMSubArch_v6t2)
+    .EndsWith("v5", Triple::ARMSubArch_v5)
+    .EndsWith("v5t", Triple::ARMSubArch_v5)
+    .EndsWith("v5te", Triple::ARMSubArch_v5te)
+    .EndsWith("v4t", Triple::ARMSubArch_v4t)
+    .EndsWith("v4", Triple::ARMSubArch_v4)
+    .Default(Triple::NoSubArch);
+}
+
 static const char *getObjectFormatTypeName(Triple::ObjectFormatType Kind) {
   switch (Kind) {
   case Triple::UnknownObjectFormat: return "";
   case Triple::COFF: return "coff";
   case Triple::ELF: return "elf";
   case Triple::MachO: return "macho";
   }
   llvm_unreachable("unknown object format type");

 
 /// \brief Construct a triple from the string representation provided.
 ///
 /// This stores the string representation and parses the various pieces into
 /// enum members.
 Triple::Triple(const Twine &Str)
     : Data(Str.str()),
       Arch(parseArch(getArchName())),
+      SubArch(parseSubArch(getArchName())),
       Vendor(parseVendor(getVendorName())),
       OS(parseOS(getOSName())),
       Environment(parseEnvironment(getEnvironmentName())),
       ObjectFormat(parseFormat(getEnvironmentName())) {
   if (ObjectFormat == Triple::UnknownObjectFormat)
     ObjectFormat = getDefaultFormat(*this);
 }
 
 /// \brief Construct a triple from string representations of the architecture,
 /// vendor, and OS.
 ///
 /// This joins each argument into a canonical string representation and parses
 /// them into enum members. It leaves the environment unknown and omits it from
 /// the string representation.
 Triple::Triple(const Twine &ArchStr, const Twine &VendorStr, const Twine &OSStr)
     : Data((ArchStr + Twine('-') + VendorStr + Twine('-') + OSStr).str()),
       Arch(parseArch(ArchStr.str())),
+      SubArch(parseSubArch(ArchStr.str())),
       Vendor(parseVendor(VendorStr.str())),
       OS(parseOS(OSStr.str())),
       Environment(), ObjectFormat(Triple::UnknownObjectFormat) {
   ObjectFormat = getDefaultFormat(*this);
 }
 
 /// \brief Construct a triple from string representations of the architecture,
 /// vendor, OS, and environment.
 ///
 /// This joins each argument into a canonical string representation and parses
 /// them into enum members.
 Triple::Triple(const Twine &ArchStr, const Twine &VendorStr, const Twine &OSStr,
                const Twine &EnvironmentStr)
     : Data((ArchStr + Twine('-') + VendorStr + Twine('-') + OSStr + Twine('-') +
             EnvironmentStr).str()),
       Arch(parseArch(ArchStr.str())),
+      SubArch(parseSubArch(ArchStr.str())),
       Vendor(parseVendor(VendorStr.str())),
       OS(parseOS(OSStr.str())),
       Environment(parseEnvironment(EnvironmentStr.str())),
       ObjectFormat(parseFormat(EnvironmentStr.str())) {
   if (ObjectFormat == Triple::UnknownObjectFormat)
     ObjectFormat = getDefaultFormat(*this);
 }
 

lib/Target/ARM/MCTargetDesc/ARMMCTargetDesc.cpp

 
 #define GET_SUBTARGETINFO_MC_DESC
 #include "ARMGenSubtargetInfo.inc"
 
 
 std::string ARM_MC::ParseARMTriple(StringRef TT, StringRef CPU) {
   Triple triple(TT);
 
-  // Set the boolean corresponding to the current target triple, or the default
-  // if one cannot be determined, to true.
-  unsigned Len = TT.size();
-  unsigned Idx = 0;
-
-  // FIXME: Enhance Triple helper class to extract ARM version.
   bool isThumb = triple.getArch() == Triple::thumb ||
                  triple.getArch() == Triple::thumbeb;
-  if (Len >= 5 && TT.substr(0, 4) == "armv")
-    Idx = 4;
-  else if (Len >= 7 && TT.substr(0, 6) == "armebv")
-    Idx = 6;
-  else if (Len >= 7 && TT.substr(0, 6) == "thumbv")
-    Idx = 6;
-  else if (Len >= 9 && TT.substr(0, 8) == "thumbebv")
-    Idx = 8;
 
   bool NoCPU = CPU == "generic" || CPU.empty();
   std::string ARMArchFeature;
-  if (Idx) {
-    unsigned SubVer = TT[Idx];
-    if (SubVer == '8') {
-      if (NoCPU)
-        // v8a: FeatureDB, FeatureFPARMv8, FeatureNEON, FeatureDSPThumb2,
-        //      FeatureMP, FeatureHWDiv, FeatureHWDivARM, FeatureTrustZone,
-        //      FeatureT2XtPk, FeatureCrypto, FeatureCRC
-        ARMArchFeature = "+v8,+db,+fp-armv8,+neon,+t2dsp,+mp,+hwdiv,+hwdiv-arm,"
-                         "+trustzone,+t2xtpk,+crypto,+crc";
-      else
-        // Use CPU to figure out the exact features
-        ARMArchFeature = "+v8";
-    } else if (SubVer == '7') {
-      if (Len >= Idx+2 && TT[Idx+1] == 'm') {
-        isThumb = true;
-        if (NoCPU)
-          // v7m: FeatureNoARM, FeatureDB, FeatureHWDiv, FeatureMClass
-          ARMArchFeature = "+v7,+noarm,+db,+hwdiv,+mclass";
-        else
-          // Use CPU to figure out the exact features.
-          ARMArchFeature = "+v7";
-      } else if (Len >= Idx+3 && TT[Idx+1] == 'e'&& TT[Idx+2] == 'm') {
-        if (NoCPU)
-          // v7em: FeatureNoARM, FeatureDB, FeatureHWDiv, FeatureDSPThumb2,
-          //       FeatureT2XtPk, FeatureMClass
-          ARMArchFeature = "+v7,+noarm,+db,+hwdiv,+t2dsp,t2xtpk,+mclass";
-        else
-          // Use CPU to figure out the exact features.
-          ARMArchFeature = "+v7";
-      } else if (Len >= Idx+2 && TT[Idx+1] == 's') {
-        if (NoCPU)
-          // v7s: FeatureNEON, FeatureDB, FeatureDSPThumb2, FeatureHasRAS
-          //      Swift
-          ARMArchFeature = "+v7,+swift,+neon,+db,+t2dsp,+ras";
-        else
-          // Use CPU to figure out the exact features.
-          ARMArchFeature = "+v7";
-      } else {
-        // v7 CPUs have lots of different feature sets. If no CPU is specified,
-        // then assume v7a (e.g. cortex-a8) feature set. Otherwise, return
-        // the "minimum" feature set and use CPU string to figure out the exact
-        // features.
-        if (NoCPU)
-          // v7a: FeatureNEON, FeatureDB, FeatureDSPThumb2, FeatureT2XtPk
-          ARMArchFeature = "+v7,+neon,+db,+t2dsp,+t2xtpk";
-        else
-          // Use CPU to figure out the exact features.
-          ARMArchFeature = "+v7";
-      }
-    } else if (SubVer == '6') {
-      if (Len >= Idx+3 && TT[Idx+1] == 't' && TT[Idx+2] == '2')
-        ARMArchFeature = "+v6t2";
-      else if (Len >= Idx+2 && TT[Idx+1] == 'm') {
-        isThumb = true;
-        if (NoCPU)
-          // v6m: FeatureNoARM, FeatureMClass
-          ARMArchFeature = "+v6m,+noarm,+mclass";
-        else
-          ARMArchFeature = "+v6";
-      } else
-        ARMArchFeature = "+v6";
-    } else if (SubVer == '5') {
-      if (Len >= Idx+3 && TT[Idx+1] == 't' && TT[Idx+2] == 'e')
-        ARMArchFeature = "+v5te";
-      else
-        ARMArchFeature = "+v5t";
-    } else if (SubVer == '4' && Len >= Idx+2 && TT[Idx+1] == 't')
-      ARMArchFeature = "+v4t";
+  switch (triple.getSubArch()) {
+  case Triple::ARMSubArch_v8:
+    if (NoCPU)
+      // v8a: FeatureDB, FeatureFPARMv8, FeatureNEON, FeatureDSPThumb2,
+      //      FeatureMP, FeatureHWDiv, FeatureHWDivARM, FeatureTrustZone,
+      //      FeatureT2XtPk, FeatureCrypto, FeatureCRC
+      ARMArchFeature = "+v8,+db,+fp-armv8,+neon,+t2dsp,+mp,+hwdiv,+hwdiv-arm,"
+                       "+trustzone,+t2xtpk,+crypto,+crc";
+    else
+      // Use CPU to figure out the exact features
+      ARMArchFeature = "+v8";
+    break;
+  case Triple::ARMSubArch_v7m:
+    isThumb = true;
+    if (NoCPU)
+      // v7m: FeatureNoARM, FeatureDB, FeatureHWDiv, FeatureMClass
+      ARMArchFeature = "+v7,+noarm,+db,+hwdiv,+mclass";
+    else
+      // Use CPU to figure out the exact features.
+      ARMArchFeature = "+v7";
+    break;
+  case Triple::ARMSubArch_v7em:
+    break;
+  case Triple::ARMSubArch_v7s:
+    if (NoCPU)
+      // v7s: FeatureNEON, FeatureDB, FeatureDSPThumb2, FeatureHasRAS
+      //      Swift
+      ARMArchFeature = "+v7,+swift,+neon,+db,+t2dsp,+ras";
+    else
+      // Use CPU to figure out the exact features.
+      ARMArchFeature = "+v7";
+    break;
+  case Triple::ARMSubArch_v7:
+    // v7 CPUs have lots of different feature sets. If no CPU is specified,
+    // then assume v7a (e.g. cortex-a8) feature set. Otherwise, return
+    // the "minimum" feature set and use CPU string to figure out the exact
+    // features.
+    if (NoCPU)
+      // v7a: FeatureNEON, FeatureDB, FeatureDSPThumb2, FeatureT2XtPk
+      ARMArchFeature = "+v7,+neon,+db,+t2dsp,+t2xtpk";
+    else
+      // Use CPU to figure out the exact features.
+      ARMArchFeature = "+v7";
+    break;
+  case Triple::ARMSubArch_v6t2:
+    ARMArchFeature = "+v6t2";
+    break;
+  case Triple::ARMSubArch_v6m:
+    isThumb = true;
+    if (NoCPU)
+      // v6m: FeatureNoARM, FeatureMClass
+      ARMArchFeature = "+v6m,+noarm,+mclass";
+    else
+    break;
+  case Triple::ARMSubArch_v6:
+    ARMArchFeature = "+v6";
+    break;
+  case Triple::ARMSubArch_v5te:
+    ARMArchFeature = "+v5te";
+    break;
+  case Triple::ARMSubArch_v5:
+    ARMArchFeature = "+v5t";
+    break;
+  case Triple::ARMSubArch_v4t:
+    ARMArchFeature = "+v4t";
+    break;
+  case Triple::NoSubArch:
+  case Triple::ARMSubArch_v4:
+    ARMArchFeature = "+v4";
+    break;
   }
 
   if (isThumb) {
     if (ARMArchFeature.empty())
       ARMArchFeature = "+thumb-mode";
     else
       ARMArchFeature += ",+thumb-mode";
   }