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

4161

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

4165

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.

4394

You should also set IsThumb and ShouldUseInlineAtomic

4536

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

4536

Are you referring to "is32Bit"? Any suggestions?

[rengolin]

lib/Basic/Targets.cpp

4536

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

4394

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

4394

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

4394

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

4394

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

4160

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

[labrinea]

lib/Basic/Targets.cpp

4160

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

4160

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

4160

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

4160

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>?

4146

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.

4165

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.

4168–4170

This should also go inside setArchInfo()

4170

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

4350

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.

4358

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.

4390

Ah, yes, this makes a lot of sense!

4401

if you cache CPUAttr, this call loses its argument.

4407

if you cache CPUAttr, this call loses its argument.

4408

if you cache CPUAttr, this call disappears.

[labrinea]

lib/Basic/Targets.cpp

33

I thought isalnum needed that.

4146

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?

4168–4170

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

4358

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

4146

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. :)

4168–4170

Good point.

4358

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

4170

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".

4176

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

4170

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.

4176

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

4170

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

4170

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.
class ARMTargetInfo : public TargetInfo {
   std::string ABI, CPU;
 
   enum {
     FP_Default,
     FP_VFP,
     FP_Neon
   } FPMath;
 
+  unsigned ArchISA;
+  unsigned ArchKind;
+  unsigned ArchProfile;
+  unsigned ArchVersion;
+
   unsigned FPU : 5;
 
+  unsigned ShouldUseInlineAtomic : 1;
   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) {
+    ArchISA     = llvm::ARMTargetParser::parseArchISA(ArchName);
+    ArchKind    = llvm::ARMTargetParser::parseArch(ArchName);
+    ArchProfile = llvm::ARMTargetParser::parseArchProfile(ArchName);
+    ArchVersion = llvm::ARMTargetParser::parseArchVersion(ArchName); 
+
+    ShouldUseInlineAtomic = (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;
+    }
+  }
+
 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;
     }
 
+    if (Triple.getSubArch() == llvm::Triple::SubArchType::NoSubArch)
+      setArchInfo("armv6j");
+    else 
+      setArchInfo(Triple.getArchName()); 
+
     // {} 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 should be updated also when calling setCPU 
+    // but if so it causes regressions
+    IsThumb = (ArchISA == llvm::ARM::IK_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");
       }
break;
           setABI("aapcs");
         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;
-
     // 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;
   }
   StringRef getABI() const override { return ABI; }
   bool setABI(const std::string &Name) override {
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;
     }
   }
 
return llvm::StringSwitch<bool>(Feature)
         .Case("arm", true)
         .Case("softfloat", SoftFloat)
         .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;
-      }
-    }
-
-    unsigned ArchKind = llvm::ARMTargetParser::parseCPUArch(Name);
-    if (ArchKind == llvm::ARM::AK_INVALID)
-      return "";
-
+  const char *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:
-      return llvm::ARMTargetParser::getCPUAttr(ArchKind);
+      CPUAttr = llvm::ARMTargetParser::getCPUAttr(ArchKind);
+      return CPUAttr ? CPUAttr : "" ;
     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)) {
+  const char *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 "";
     }
   }
   bool setCPU(const std::string &Name) override {
-    if (!getCPUDefineSuffix(Name))
+    unsigned ArchKind = llvm::ARMTargetParser::parseCPUArch(Name);
+
+    if (ArchKind == llvm::ARM::AK_INVALID)
       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(llvm::ARMTargetParser::getArchName(ArchKind));
 
     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;
+  bool supportsThumb(StringRef CPUAttr) const {
+    return CPUAttr.count('T') || ArchVersion >= 6;
+  }
+  bool supportsThumb2(StringRef CPUAttr) const {
+    return CPUAttr.equals("6T2") || ArchVersion >= 7;
   }
   void getTargetDefines(const LangOptions &Opts,
                         MacroBuilder &Builder) const override {
+    StringRef CPUAttr    = getCPUAttr();
+    StringRef CPUProfile = getCPUProfile();
+
     // 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 + "__");
+    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(CPUAttr))
       Builder.defineMacro("__ARM_ARCH_ISA_THUMB", "2");
-    else if (supportsThumb(ArchName, CPUArch, CPUArchVer))
+    else if (supportsThumb(CPUAttr))
       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) {
       Builder.defineMacro("__THUMBEL__");
       Builder.defineMacro("__thumb__");
-      if (supportsThumb2(ArchName, CPUArch, CPUArchVer))
+      if (supportsThumb2(CPUAttr))
         Builder.defineMacro("__thumb2__");
     }
     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(CPUAttr));
+    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; }