code refactoring on ARMTargetInfo class

[labrinea]

Summary

Refactored ARMTargetInfo in to use existing API from llvm/lib/Support/TargetParser.cpp in order to extract target specific information.

[rengolin]

Hi Alexandros,

Thanks for the patch. I'm really happy that we can now change everything else and not need to change the TargetParser any more. This means we're stabilising the interface, and soon will be time to move it inside the new Tuple class.

About your patch, it's overall good, but I have a few minor comments inline.

cheers,
--renato

lib/Basic/Targets.cpp

4303

These could be cached, too, as members of the class.

4307

What if this goes wrong?

I guess all other methods in the class will start to return rubbish, but then again, that's what it currently does.

Maybe a future patch to cover the cases where the values are XX_INVALID on all the get methods and print some warning/error.

Also, you might want to extract this lines (and IsThumb / Atomic below) into a private method that both the constructor and setCPU call.

4498

You should also set IsThumb and ShouldUseInlineAtomic

4630

This variable name is odd...

[labrinea]

In D10839#197340, @rengolin wrote:

Hi Alexandros,

Thanks for the patch. I'm really happy that we can now change everything else and not need to change the TargetParser any more. This means we're stabilising the interface, and soon will be time to move it inside the new Tuple class.

About your patch, it's overall good, but I have a few minor comments inline.

cheers,
--renato

Thanks for the feedback. I have already made some changes and I will upload a new patch soon. Regarding the odd variable name? (it was not quite clear what I should change)

lib/Basic/Targets.cpp

4630

Are you referring to "is32Bit"? Any suggestions?

[rengolin]

lib/Basic/Targets.cpp

4630

Yes. I'm not sure what this is trying to achieve, TBH, but getting "all 32-bit ARM architectures" is not. :)

Though, since this was there before, I guess it's ok to keep it for now. Feel free to add a FIXME comment.

[labrinea]

lib/Basic/Targets.cpp

4498

Setting IsThumb in setCPU causes regressions. It doesn't make sense to me but setCPU changes the ISA from thumb to arm.

[rengolin]

lib/Basic/Targets.cpp

4498

Right, this is a major bug and needs to be addressed. You shouldn't mix TargetParser changes with bug fixes, but would be good to add a FIXME comment and open a bug in bugzilla to describe the problem.

What about ShouldUseInlineAtomic?

[labrinea]

lib/Basic/Targets.cpp

4498

In the current implementation ShouldUseInlineAtomic is checking llvm::triple which is set in the constructor. Apparently, setCPU checks MaxAtomicInlineWidth (which is also set in the constructor depending on the value of ShouldUseInlineAtomic), thus takes into account the ISA from llvm::triple.

[rengolin]

lib/Basic/Targets.cpp

4498

It uses the ISA and the ArchVersion, which can change here.

[labrinea]

made some changes:

• according to review suggestions
• getCPUAttr() was causing regressions when returning nullptr
• the API from TargetParser was causing regressions when llvm::triple subArch was not specified

[rengolin]

lib/Basic/Targets.cpp

4302

This seems quite arbitrary... Couldn't you return an empty string?

[labrinea]

lib/Basic/Targets.cpp

4302

If no subArch is specified then setArchInfo() cannot do much with an empty string. ArcKind will be set to invalid and getCPUAttr() will return an empty string. A regression will appear since ARM_ARCH_6J won't be defined as expected. That ("armv6j") should be the corresponding subArch for the default CPU ("arm1136j-s") specified in the constructor.

[rengolin]

lib/Basic/Targets.cpp

4302

Then, use the default value in the constructor (CPU) instead, to figure out the arch.

Don't just hard-code yet another temporarily matching value.

[labrinea]

lib/Basic/Targets.cpp

4302

If it is more maintainable instead of setArchInfo("armv6j") we could call setCPU(CPU) (where cpu is "arm1136j-s" by default) in case subArch is not defined.

[rengolin]

lib/Basic/Targets.cpp

4302

or you could use TargetParser::parseCPUArch(CPU);

Anything bur more hardcoded values.

[labrinea]

Ok, fair enough. Thanks again for the feedback :) I am currently trying to replace some of the if-else statements in getDefaultFeatures() that extract FPU information based on the CPU. I am adding a helper function in TargetParser for that. It maps CPU names to FPUKinds. Shall I commit the current patch first, or altogether?

[rengolin]

In D10839#198411, @labrinea wrote:

Ok, fair enough. Thanks again for the feedback :) I am currently trying to replace some of the if-else statements in getDefaultFeatures() that extract FPU information based on the CPU. I am adding a helper function in TargetParser for that. It maps CPU names to FPUKinds. Shall I commit the current patch first, or altogether?

I think you should commit this small fix now, and then refactor it later if needed, with your next patch.

Thanks!

[labrinea]

Updated, shall I commit it?

[rengolin]

Yup, looks good to me, now. Thanks!

--renato

[labrinea]

The previous patch was causing regressions because ShouldUseInlineAtomic was set incorrectly when subArch was not specified. My mistake that I did not notice it before the commit. Again, the fact that setCPU() can change the Arch that was previously defined by llvm::triple doesn't make sense to me. That is the reason that ShouldUseInlineAtomic and IsThumb are initialized in the constructor and never change again. Anyway, I added some FIXME labels. Shall I commit?

[rengolin]

If this makes the tests pass, I'm ok with it, for now. But that part is getting uglier. We should understand what's causing the regressions and fix it as soon as possible.

[labrinea]

I agree.

[rengolin]

Thanks, please commit.

[rengolin]

Hi Alexandros,

Thanks again for going through this. I have a number of comments, mainly regards organization. This class is a bit messy, and since we're touching most of it's caching logic, I think it's fair if we do a bit of cleaning up.

cheers,
--renato

lib/Basic/Targets.cpp

33

Why do you need to include <locale>?

4288

Since we're setting up the default CPU on the Arch, we don't need it to be initialised here.

Also, "arm1136" looks like the wrong value here, since the default CPU for ARM when nothing is known is "arm7tdmi".

I risk to say that this value was never taken into consideration because the architecture was never empty (errors dealt with before), so I'd leave CPU uninitialized here, and assert down there if CPU == nullptr.

4307

Maybe we should have two setArchInfo():

setArchInfo(ArchName);

Called here, and set when the Arch changes. And:

setArchInfo(CPUName);

set from setCPU() below. This version doesn't change the Arch, only the CPU affected parts of the cached parameters.

The Arch version should call the CPU version to remove redundancies in the code.

4307–4308

This should also go inside setArchInfo()

4312

ShouldUseInlineAtomic must be set up inside setAtomic(), too.

4494

Ah, yes, this makes a lot of sense!

4494

I'm not why you're changing the name of this function, but it seems it's rather a private matter (no use outside of this file).

Since we're caching all properties of the target, I suggest you move this to private and call it on setArchInfo() only and keep a local copy of it.

4496

This validation doesn't belong here. If ARMTargetParser returned a valid CPU name, the name is valid.

If the name is wrong, ARMTargetParser is wrong and should be fixed instead.

4502

if you cache CPUAttr, this call loses its argument.

4504

if you cache CPUAttr, this call loses its argument.

4505

if you cache CPUAttr, this call disappears.

[labrinea]

lib/Basic/Targets.cpp

33

I thought isalnum needed that.

4288

The value "arm1136j-s" was taken into consideration. A regression will appear if we don't set it, since ARM_ARCH_6J won't be defined as expected (tools/clang/test/Preprocessor/init.c). I don't like this hard-coded string either but it was there before my patch. Do you think we should change the test?

4307–4308

IsThumb depends on ArchISA. ArchISA should be initialized based on triple and never change. Updating IsThumb in setArchInfo() will not change its value.

4496

That's the assertion I introduced to catch the buildbot error (whitespace expected after macro definition). I am still surprised that it passed the test this time since the only thing I added is three cases to the switch clause (ARMV6HL, AK_ARMV7L, AK_ARMV7HL). Those were the only cases of illegal characters ( '-' ) in CPUAttr not handled.

[rengolin]

lib/Basic/Targets.cpp

4288

No. Add another FIXME outlining the problem, hinting where to fix it properly. This patch is already too big and long lasting for its own good. :)

4307–4308

Good point.

4496

Regardless, this is not the place to check for this kind of problem.

Maybe, on a separate patch, we can add that check to ARMTargetParser, which is the right place.

Also, using std::string is a bit heavy handed for this task.

[labrinea]

lib/Basic/Targets.cpp

4312

Moving it there triggers regression (tools/clang/test/CodeGen/atomics-inlining.c). When triple does not define a subArch, (and therefore llvm::ARMTargetParser::getDefaultCPU(ArchName) returns null), the value of ShouldUseInlineAtomic should be false according to the logic before my patch. That's because ShouldUseInlineAtomic was parsing the arch name and returned true only if archName started with "armv" or "thumbv".

4318

If we are finally keeping the hard-coded string for CPU with a FIXME label, there is no point in having setArchInfo(ArchName). It is redundant. We still have to call setArchInfo(CPU) to avoid the regressions.

[rengolin]

lib/Basic/Targets.cpp

4312

But by the time you run setAtomic() you already have Arch, CPU and everything. And if you don't, then check for nullptr and set atomic to false.

4318

True. And since this is the constructor, and the Arch is not supposed to change, it should be fine to keep the Arch part of it, here.

[labrinea]

lib/Basic/Targets.cpp

4312

The tricky part is that these variables (Arch, CPU, etc..) are set both in the if and the else path, regardless if the triple specifies a sub arch or not. What I could do is to cache DefaultCPU, which is set by llvm::ARMTargetParser::getDefaultCPU(ArchName), and test that for nullptr value in setAtomic().

[rengolin]

lib/Basic/Targets.cpp

4312

Yes, that could work.

[labrinea]

I have updated the revision in case phabricator did not send any notification.

[labrinea]

Is it approved for commit?

[rengolin]

Not yet, let me check over all concerns again.

[rengolin]

Hi Alexandros,

This now looks good to go. Thanks for the patience! :)

Have you tested on ARMv7, self-hosting? IIRC, that's where it was failing last time.

cheers,
--renato

lib/Basic/Targets.cpp

 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/MC/MCSectionMachO.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/TargetParser.h"
 #include <algorithm>
 #include <memory>
 using namespace clang;
 
 //===----------------------------------------------------------------------===//
 //  Common code shared among targets.
 //===----------------------------------------------------------------------===//
 
 /// DefineStd - Define a macro name and standard variants.  For example if
 /// MacroName is "unix", then this will define "__unix", "__unix__", and "unix"
 /// when in GNU mode.
static bool FPUModeIsVFP(FPUMode Mode) {
     return Mode & (VFP2FPU | VFP3FPU | VFP4FPU | NeonFPU | FPARMV8);
   }
 
   static const TargetInfo::GCCRegAlias GCCRegAliases[];
   static const char * const GCCRegNames[];
 
   std::string ABI, CPU;
 
+  StringRef DefaultCPU;
+  StringRef CPUProfile;
+  StringRef CPUAttr;
+
   enum {
     FP_Default,
     FP_VFP,
     FP_Neon
   } FPMath;
 
+  unsigned ArchISA;
+  unsigned ArchKind;
+  unsigned ArchProfile;
+  unsigned ArchVersion;
+
   unsigned FPU : 5;
 
   unsigned IsAAPCS : 1;
-  unsigned IsThumb : 1;
   unsigned HWDiv : 2;
 
   // Initialized via features.
   unsigned SoftFloat : 1;
   unsigned SoftFloatABI : 1;
 
   unsigned CRC : 1;
   unsigned Crypto : 1;
 
   // ACLE 6.5.1 Hardware floating point
   enum {
     HW_FP_HP = (1 << 1), /// half (16-bit)
     HW_FP_SP = (1 << 2), /// single (32-bit)
     HW_FP_DP = (1 << 3), /// double (64-bit)
   };
   uint32_t HW_FP;
 
   static const Builtin::Info BuiltinInfo[];
 
-  static bool shouldUseInlineAtomic(const llvm::Triple &T) {
-    StringRef ArchName = T.getArchName();
-    if (T.getArch() == llvm::Triple::arm ||
-        T.getArch() == llvm::Triple::armeb) {
-      StringRef VersionStr;
-      if (ArchName.startswith("armv"))
-        VersionStr = ArchName.substr(4, 1);
-      else if (ArchName.startswith("armebv"))
-        VersionStr = ArchName.substr(6, 1);
-      else
-        return false;
-      unsigned Version;
-      if (VersionStr.getAsInteger(10, Version))
-        return false;
-      return Version >= 6;
-    }
-    assert(T.getArch() == llvm::Triple::thumb ||
-           T.getArch() == llvm::Triple::thumbeb);
-    StringRef VersionStr;
-    if (ArchName.startswith("thumbv"))
-      VersionStr = ArchName.substr(6, 1);
-    else if (ArchName.startswith("thumbebv"))
-      VersionStr = ArchName.substr(8, 1);
-    else
-      return false;
-    unsigned Version;
-    if (VersionStr.getAsInteger(10, Version))
-      return false;
-    return Version >= 7;
-  }
-
   void setABIAAPCS() {
     IsAAPCS = true;
 
     DoubleAlign = LongLongAlign = LongDoubleAlign = SuitableAlign = 64;
     const llvm::Triple &T = getTriple();
 
     // size_t is unsigned long on MachO-derived environments, NetBSD and Bitrig.
     if (T.isOSBinFormatMachO() || T.getOS() == llvm::Triple::NetBSD ||
else
       DescriptionString =
           BigEndian
               ? "E-m:e-p:32:32-f64:32:64-v64:32:64-v128:32:128-a:0:32-n32-S32"
               : "e-m:e-p:32:32-f64:32:64-v64:32:64-v128:32:128-a:0:32-n32-S32";
 
     // FIXME: Override "preferred align" for double and long long.
   }
 
+  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);
+  }
+
+  void setArchInfo(StringRef CPU) {
+    StringRef SubArch;
+
+    // cache TargetParser info
+    ArchKind    = llvm::ARMTargetParser::parseCPUArch(CPU);
+    SubArch     = llvm::ARMTargetParser::getSubArch(ArchKind);
+    ArchProfile = llvm::ARMTargetParser::parseArchProfile(SubArch);
+    ArchVersion = llvm::ARMTargetParser::parseArchVersion(SubArch);
+
+    // cache CPU related strings
+    CPUAttr    = getCPUAttr();
+    CPUProfile = getCPUProfile();
+  }
+
+  void setAtomic() {
+    // when triple does not specify a sub arch, 
+    // then we are not using inline atomics
+    bool ShouldUseInlineAtomic = DefaultCPU.empty() ? 
+                                 false :
+                   (ArchISA == llvm::ARM::IK_ARM   && ArchVersion >= 6) ||
+                   (ArchISA == llvm::ARM::IK_THUMB && ArchVersion >= 7);
+    // Cortex M does not support 8 byte atomics, while general Thumb2 does. 
+    if (ArchProfile == llvm::ARM::PK_M) {
+      MaxAtomicPromoteWidth = 32;
+      if (ShouldUseInlineAtomic)
+        MaxAtomicInlineWidth = 32;
+    }
+    else {
+      MaxAtomicPromoteWidth = 64;
+      if (ShouldUseInlineAtomic)
+        MaxAtomicInlineWidth = 64;
+    } 
+  }
+
+  bool isThumb() const {
+    return (ArchISA == llvm::ARM::IK_THUMB);
+  }
+
+  bool supportsThumb() const {
+    return CPUAttr.count('T') || ArchVersion >= 6;
+  }
+
+  bool supportsThumb2() const {
+    return CPUAttr.equals("6T2") || ArchVersion >= 7;
+  }
+
+  StringRef getDefaultCPU(StringRef ArchName) const {
+    const char *DefaultCPU = llvm::ARMTargetParser::getDefaultCPU(ArchName);
+    return DefaultCPU ? DefaultCPU : "";
+  }
+
+  StringRef getCPUAttr() const {
+    const char *CPUAttr;
+    // For most sub-arches, the build attribute CPU name is enough.
+    // For Cortex variants, it's slightly different.
+    switch(ArchKind) {
+    default:
+      CPUAttr = llvm::ARMTargetParser::getCPUAttr(ArchKind);
+      return CPUAttr ? CPUAttr : "";      
+    case llvm::ARM::AK_ARMV6M:
+    case llvm::ARM::AK_ARMV6SM:
+    case llvm::ARM::AK_ARMV6HL:
+      return "6M";
+    case llvm::ARM::AK_ARMV7:
+    case llvm::ARM::AK_ARMV7A:
+    case llvm::ARM::AK_ARMV7S:
+    case llvm::ARM::AK_ARMV7L:
+    case llvm::ARM::AK_ARMV7HL:
+      return "7A";
+    case llvm::ARM::AK_ARMV7R:
+      return "7R";
+    case llvm::ARM::AK_ARMV7M:
+      return "7M";
+    case llvm::ARM::AK_ARMV7EM:
+      return "7EM";
+    case llvm::ARM::AK_ARMV8A:
+      return "8A";
+    case llvm::ARM::AK_ARMV8_1A:
+      return "8_1A";
+    }
+  }
+
+  StringRef getCPUProfile() const {
+    switch(ArchProfile) {
+    case llvm::ARM::PK_A:
+      return "A";
+    case llvm::ARM::PK_R:
+      return "R";
+    case llvm::ARM::PK_M:
+      return "M";
+    default:
+      return "";
+    }
+  }
+
 public:
   ARMTargetInfo(const llvm::Triple &Triple, bool IsBigEndian)
       : TargetInfo(Triple), CPU("arm1136j-s"), FPMath(FP_Default),
         IsAAPCS(true), HW_FP(0) {
     BigEndian = IsBigEndian;
 
     switch (getTriple().getOS()) {
     case llvm::Triple::NetBSD:
       PtrDiffType = SignedLong;
       break;
     default:
       PtrDiffType = SignedInt;
       break;
     }
    
+    // cache arch related info
+    setArchInfo();
+  
     // {} in inline assembly are neon specifiers, not assembly variant
     // specifiers.
     NoAsmVariants = true;
 
-    // FIXME: Should we just treat this as a feature?
-    IsThumb = getTriple().getArchName().startswith("thumb");
-
     // FIXME: This duplicates code from the driver that sets the -target-abi
     // option - this code is used if -target-abi isn't passed and should
     // be unified in some way.
     if (Triple.isOSBinFormatMachO()) {
       // The backend is hardwired to assume AAPCS for M-class processors, ensure
       // the frontend matches that.
       if (Triple.getEnvironment() == llvm::Triple::EABI ||
           Triple.getOS() == llvm::Triple::UnknownOS ||
           StringRef(CPU).startswith("cortex-m")) {
         setABI("aapcs");
       } else {
         setABI("apcs-gnu");
       }
     } else if (Triple.isOSWindows()) {
       // FIXME: this is invalid for WindowsCE
       setABI("aapcs");
     } else {
       // Select the default based on the platform.
       switch (Triple.getEnvironment()) {
break;
         break;
       }
     }
 
     // ARM targets default to using the ARM C++ ABI.
     TheCXXABI.set(TargetCXXABI::GenericARM);
 
     // ARM has atomics up to 8 bytes
-    MaxAtomicPromoteWidth = 64;
-    if (shouldUseInlineAtomic(getTriple()))
-      MaxAtomicInlineWidth = 64;
+    setAtomic();
 
     // Do force alignment of members that follow zero length bitfields.  If
     // the alignment of the zero-length bitfield is greater than the member
     // that follows it, `bar', `bar' will be aligned as the  type of the
     // zero length bitfield.
     UseZeroLengthBitfieldAlignment = true;
   }
 
if (Name == "aapcs" || Name == "aapcs-vfp" || Name == "aapcs-linux") {
       setABIAAPCS();
       return true;
     }
     return false;
   }
 
   // FIXME: This should be based on Arch attributes, not CPU names.
   void getDefaultFeatures(llvm::StringMap<bool> &Features) const override {
-    StringRef ArchName = getTriple().getArchName();
-    unsigned ArchKind = llvm::ARMTargetParser::parseArch(ArchName);
-    bool IsV8 = (ArchKind == llvm::ARM::AK_ARMV8A ||
-                 ArchKind == llvm::ARM::AK_ARMV8_1A);
-
     if (CPU == "arm1136jf-s" || CPU == "arm1176jzf-s" || CPU == "mpcore")
       Features["vfp2"] = true;
     else if (CPU == "cortex-a8" || CPU == "cortex-a9") {
       Features["vfp3"] = true;
       Features["neon"] = true;
     }
     else if (CPU == "cortex-a5") {
       Features["vfp4"] = true;
       Features["neon"] = true;
     } else if (CPU == "swift" || CPU == "cortex-a7" ||
                CPU == "cortex-a12" || CPU == "cortex-a15" ||
                CPU == "cortex-a17" || CPU == "krait") {
       Features["vfp4"] = true;
       Features["neon"] = true;
       Features["hwdiv"] = true;
       Features["hwdiv-arm"] = true;
     } else if (CPU == "cyclone" || CPU == "cortex-a53" || CPU == "cortex-a57" ||
                CPU == "cortex-a72") {
       Features["fp-armv8"] = true;
       Features["neon"] = true;
       Features["hwdiv"] = true;
       Features["hwdiv-arm"] = true;
       Features["crc"] = true;
       Features["crypto"] = true;
-    } else if (CPU == "cortex-r5" || CPU == "cortex-r7" || IsV8) {
+    } else if (CPU == "cortex-r5" || CPU == "cortex-r7" || ArchVersion == 8) {
       Features["hwdiv"] = true;
       Features["hwdiv-arm"] = true;
     } else if (CPU == "cortex-m3" || CPU == "cortex-m4" || CPU == "cortex-m7" ||
                CPU == "sc300" || CPU == "cortex-r4" || CPU == "cortex-r4f") {
       Features["hwdiv"] = true;
     }
   }
 
bool handleTargetFeatures(std::vector<std::string> &Features,
 
     return true;
   }
 
   bool hasFeature(StringRef Feature) const override {
     return llvm::StringSwitch<bool>(Feature)
         .Case("arm", true)
         .Case("softfloat", SoftFloat)
-        .Case("thumb", IsThumb)
+        .Case("thumb", isThumb())
         .Case("neon", (FPU & NeonFPU) && !SoftFloat)
         .Case("hwdiv", HWDiv & HWDivThumb)
         .Case("hwdiv-arm", HWDiv & HWDivARM)
         .Default(false);
   }
-  const char *getCPUDefineSuffix(StringRef Name) const {
-    if(Name == "generic") {
-      auto subarch = getTriple().getSubArch();
-      switch (subarch) {
-        case llvm::Triple::SubArchType::ARMSubArch_v8_1a: 
-          return "8_1A";
-        default:
-          break;
-      }
-    }
 
+  bool setCPU(const std::string &Name) override {
     unsigned ArchKind = llvm::ARMTargetParser::parseCPUArch(Name);
     if (ArchKind == llvm::ARM::AK_INVALID)
-      return "";
-
-    // For most sub-arches, the build attribute CPU name is enough.
-    // For Cortex variants, it's slightly different.
-    switch(ArchKind) {
-    default:
-      return llvm::ARMTargetParser::getCPUAttr(ArchKind);
-    case llvm::ARM::AK_ARMV6M:
-    case llvm::ARM::AK_ARMV6SM:
-      return "6M";
-    case llvm::ARM::AK_ARMV7:
-    case llvm::ARM::AK_ARMV7A:
-    case llvm::ARM::AK_ARMV7S:
-      return "7A";
-    case llvm::ARM::AK_ARMV7R:
-      return "7R";
-    case llvm::ARM::AK_ARMV7M:
-      return "7M";
-    case llvm::ARM::AK_ARMV7EM:
-      return "7EM";
-    case llvm::ARM::AK_ARMV8A:
-      return "8A";
-    case llvm::ARM::AK_ARMV8_1A:
-      return "8_1A";
-    }
-  }
-  const char *getCPUProfile(StringRef Name) const {
-    if(Name == "generic") {
-      auto subarch = getTriple().getSubArch();
-      switch (subarch) {
-        case llvm::Triple::SubArchType::ARMSubArch_v8_1a: 
-          return "A";
-        default:
-          break;
-      }
-    }
-
-    unsigned CPUArch = llvm::ARMTargetParser::parseCPUArch(Name);
-    if (CPUArch == llvm::ARM::AK_INVALID)
-      return "";
-
-    StringRef ArchName = llvm::ARMTargetParser::getArchName(CPUArch);
-    switch(llvm::ARMTargetParser::parseArchProfile(ArchName)) {
-      case llvm::ARM::PK_A:
-        return "A";
-      case llvm::ARM::PK_R:
-        return "R";
-      case llvm::ARM::PK_M:
-        return "M";
-      default:
-        return "";
-    }
-  }
-  bool setCPU(const std::string &Name) override {
-    if (!getCPUDefineSuffix(Name))
       return false;
-
-    // Cortex M does not support 8 byte atomics, while general Thumb2 does.
-    StringRef Profile = getCPUProfile(Name);
-    if (Profile == "M" && MaxAtomicInlineWidth) {
-      MaxAtomicPromoteWidth = 32;
-      MaxAtomicInlineWidth = 32;
-    }
-
+    setArchInfo(Name);
+    setAtomic();
     CPU = Name;
     return true;
   }
+
   bool setFPMath(StringRef Name) override;
-  bool supportsThumb(StringRef ArchName, StringRef CPUArch,
-                     unsigned CPUArchVer) const {
-    return CPUArchVer >= 7 || (CPUArch.find('T') != StringRef::npos) ||
-           (CPUArch.find('M') != StringRef::npos);
-  }
-  bool supportsThumb2(StringRef ArchName, StringRef CPUArch,
-                      unsigned CPUArchVer) const {
-    // We check both CPUArchVer and ArchName because when only triple is
-    // specified, the default CPU is arm1136j-s.
-    return ArchName.endswith("v6t2") || ArchName.endswith("v7") ||
-           ArchName.endswith("v8.1a") ||
-           ArchName.endswith("v8") || CPUArch == "6T2" || CPUArchVer >= 7;
-  }
+
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override {
     // Target identification.
     Builder.defineMacro("__arm");
     Builder.defineMacro("__arm__");
 
     // Target properties.
     Builder.defineMacro("__REGISTER_PREFIX__", "");
-
-    StringRef CPUArch = getCPUDefineSuffix(CPU);
-    unsigned int CPUArchVer;
-    if (CPUArch.substr(0, 1).getAsInteger<unsigned int>(10, CPUArchVer))
-      llvm_unreachable("Invalid char for architecture version number");
-    Builder.defineMacro("__ARM_ARCH_" + CPUArch + "__");
+    if (!CPUAttr.empty())
+      Builder.defineMacro("__ARM_ARCH_" + CPUAttr + "__");
 
     // ACLE 6.4.1 ARM/Thumb instruction set architecture
-    StringRef CPUProfile = getCPUProfile(CPU);
-    StringRef ArchName = getTriple().getArchName();
-
     // __ARM_ARCH is defined as an integer value indicating the current ARM ISA
-    Builder.defineMacro("__ARM_ARCH", CPUArch.substr(0, 1));
-    if (CPUArch[0] >= '8') {
+    Builder.defineMacro("__ARM_ARCH", std::to_string(ArchVersion));
+    if (ArchVersion >= 8) {
       Builder.defineMacro("__ARM_FEATURE_NUMERIC_MAXMIN");
       Builder.defineMacro("__ARM_FEATURE_DIRECTED_ROUNDING");
     }
 
     // __ARM_ARCH_ISA_ARM is defined to 1 if the core supports the ARM ISA.  It
     // is not defined for the M-profile.
     // NOTE that the deffault profile is assumed to be 'A'
     if (CPUProfile.empty() || CPUProfile != "M")
       Builder.defineMacro("__ARM_ARCH_ISA_ARM", "1");
 
     // __ARM_ARCH_ISA_THUMB is defined to 1 if the core supporst the original
     // Thumb ISA (including v6-M).  It is set to 2 if the core supports the
     // Thumb-2 ISA as found in the v6T2 architecture and all v7 architecture.
-    if (supportsThumb2(ArchName, CPUArch, CPUArchVer))
+    if (supportsThumb2())
       Builder.defineMacro("__ARM_ARCH_ISA_THUMB", "2");
-    else if (supportsThumb(ArchName, CPUArch, CPUArchVer))
+    else if (supportsThumb())
       Builder.defineMacro("__ARM_ARCH_ISA_THUMB", "1");
 
     // __ARM_32BIT_STATE is defined to 1 if code is being generated for a 32-bit
     // instruction set such as ARM or Thumb.
     Builder.defineMacro("__ARM_32BIT_STATE", "1");
 
     // ACLE 6.4.2 Architectural Profile (A, R, M or pre-Cortex)
 
     // __ARM_ARCH_PROFILE is defined as 'A', 'R', 'M' or 'S', or unset.
     if (!CPUProfile.empty())
       Builder.defineMacro("__ARM_ARCH_PROFILE", "'" + CPUProfile + "'");
 
     // ACLE 6.5.1 Hardware Floating Point
     if (HW_FP)
       Builder.defineMacro("__ARM_FP", "0x" + llvm::utohexstr(HW_FP));
 
     // ACLE predefines.
     Builder.defineMacro("__ARM_ACLE", "200");
 
     // Subtarget options.
 
     // FIXME: It's more complicated than this and we don't really support
     // interworking.
     // Windows on ARM does not "support" interworking
-    if (5 <= CPUArchVer && CPUArchVer <= 8 && !getTriple().isOSWindows())
+    if (5 <= ArchVersion && ArchVersion <= 8 && !getTriple().isOSWindows())
       Builder.defineMacro("__THUMB_INTERWORK__");
 
     if (ABI == "aapcs" || ABI == "aapcs-linux" || ABI == "aapcs-vfp") {
       // Embedded targets on Darwin follow AAPCS, but not EABI.
       // Windows on ARM follows AAPCS VFP, but does not conform to EABI.
       if (!getTriple().isOSDarwin() && !getTriple().isOSWindows())
         Builder.defineMacro("__ARM_EABI__");
       Builder.defineMacro("__ARM_PCS", "1");
 
       if ((!SoftFloat && !SoftFloatABI) || ABI == "aapcs-vfp")
         Builder.defineMacro("__ARM_PCS_VFP", "1");
     }
 
     if (SoftFloat)
       Builder.defineMacro("__SOFTFP__");
 
     if (CPU == "xscale")
       Builder.defineMacro("__XSCALE__");
 
-    if (IsThumb) {
+    if (isThumb()) {
       Builder.defineMacro("__THUMBEL__");
       Builder.defineMacro("__thumb__");
-      if (supportsThumb2(ArchName, CPUArch, CPUArchVer))
+      if (supportsThumb2())
         Builder.defineMacro("__thumb2__");
     }
-    if (((HWDiv & HWDivThumb) && IsThumb) || ((HWDiv & HWDivARM) && !IsThumb))
+    if (((HWDiv & HWDivThumb) && isThumb()) || ((HWDiv & HWDivARM) && !isThumb()))
       Builder.defineMacro("__ARM_ARCH_EXT_IDIV__", "1");
 
     // Note, this is always on in gcc, even though it doesn't make sense.
     Builder.defineMacro("__APCS_32__");
 
     if (FPUModeIsVFP((FPUMode) FPU)) {
       Builder.defineMacro("__VFP_FP__");
       if (FPU & VFP2FPU)
         Builder.defineMacro("__ARM_VFPV2__");
       if (FPU & VFP3FPU)
         Builder.defineMacro("__ARM_VFPV3__");
       if (FPU & VFP4FPU)
         Builder.defineMacro("__ARM_VFPV4__");
     }
 
     // This only gets set when Neon instructions are actually available, unlike
     // the VFP define, hence the soft float and arch check. This is subtly
     // different from gcc, we follow the intent which was that it should be set
     // when Neon instructions are actually available.
-    if ((FPU & NeonFPU) && !SoftFloat && CPUArchVer >= 7) {
+    if ((FPU & NeonFPU) && !SoftFloat && ArchVersion >= 7) {
       Builder.defineMacro("__ARM_NEON");
       Builder.defineMacro("__ARM_NEON__");
     }
 
     Builder.defineMacro("__ARM_SIZEOF_WCHAR_T",
                         Opts.ShortWChar ? "2" : "4");
 
     Builder.defineMacro("__ARM_SIZEOF_MINIMAL_ENUM",
                         Opts.ShortEnums ? "1" : "4");
 
     if (CRC)
       Builder.defineMacro("__ARM_FEATURE_CRC32");
 
     if (Crypto)
       Builder.defineMacro("__ARM_FEATURE_CRYPTO");
 
-    if (CPUArchVer >= 6 && CPUArch != "6M") {
+    if (ArchVersion >= 6 && CPUAttr != "6M") {
       Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_1");
       Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_2");
       Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_4");
       Builder.defineMacro("__GCC_HAVE_SYNC_COMPARE_AND_SWAP_8");
     }
 
-    bool is5EOrAbove = (CPUArchVer >= 6 ||
-                        (CPUArchVer == 5 &&
-                         CPUArch.find('E') != StringRef::npos));
-    bool is32Bit = (!IsThumb || supportsThumb2(ArchName, CPUArch, CPUArchVer));
-    if (is5EOrAbove && is32Bit && (CPUProfile != "M" || CPUArch  == "7EM"))
+    bool is5EOrAbove = (ArchVersion >= 6 ||
+                       (ArchVersion == 5 && CPUAttr.count('E')));
+    // FIXME: We are not getting all 32-bit ARM architectures
+    bool is32Bit = (!isThumb() || supportsThumb2());
+    if (is5EOrAbove && is32Bit && (CPUProfile != "M" || CPUAttr  == "7EM"))
       Builder.defineMacro("__ARM_FEATURE_DSP");
   }
+
   void getTargetBuiltins(const Builtin::Info *&Records,
                          unsigned &NumRecords) const override {
     Records = BuiltinInfo;
     NumRecords = clang::ARM::LastTSBuiltin-Builtin::FirstTSBuiltin;
   }
   bool isCLZForZeroUndef() const override { return false; }
   BuiltinVaListKind getBuiltinVaListKind() const override {
     return IsAAPCS ? AAPCSABIBuiltinVaList : TargetInfo::VoidPtrBuiltinVaList;