[PATCH] D10839: code refactoring on ARMTargetInfo class

[Renato Golin]

rengolin added a comment.

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


================
Comment at: lib/Basic/Targets.cpp:33
@@ -32,2 +32,3 @@
 #include <memory>
+#include <locale>
 using namespace clang;
----------------
Why do you need to include <locale>?

================
Comment at: lib/Basic/Targets.cpp:4198
@@ -4199,3 +4197,3 @@
   ARMTargetInfo(const llvm::Triple &Triple, bool IsBigEndian)
       : TargetInfo(Triple), CPU("arm1136j-s"), FPMath(FP_Default),
         IsAAPCS(true), HW_FP(0) {
----------------
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.

================
Comment at: lib/Basic/Targets.cpp:4217
@@ +4216,3 @@
+    ArchISA    = llvm::ARMTargetParser::parseArchISA(ArchName);
+    DefaultCPU = llvm::ARMTargetParser::getDefaultCPU(ArchName);
+
----------------
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.

================
Comment at: lib/Basic/Targets.cpp:4222
@@ +4221,3 @@
+      setArchInfo(DefaultCPU);
+      ShouldUseInlineAtomic = (ArchISA == llvm::ARM::IK_ARM &&
+                                          ArchVersion >= 6) ||
----------------
ShouldUseInlineAtomic must be set up inside setAtomic(), too.

================
Comment at: lib/Basic/Targets.cpp:4236
@@ -4216,4 +4235,3 @@
 
-    // FIXME: Should we just treat this as a feature?
-    IsThumb = getTriple().getArchName().startswith("thumb");
+    IsThumb = (ArchISA == llvm::ARM::IK_THUMB);
 
----------------
This should also go inside setArchInfo()

================
Comment at: lib/Basic/Targets.cpp:4421
@@ -4423,1 +4420,3 @@
 
+  StringRef getCPUAttr() const {
+    const char *CPUAttr;
----------------
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.

================
Comment at: lib/Basic/Targets.cpp:4430
@@ +4429,3 @@
+        return "";
+      for(char c : std::string(CPUAttr))
+        assert( (std::isalnum(c) || c == '_') &&
----------------
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.

================
Comment at: lib/Basic/Targets.cpp:4472
@@ -4477,1 +4471,3 @@
+    unsigned ArchKind = llvm::ARMTargetParser::parseCPUArch(Name);
+    if (ArchKind == llvm::ARM::AK_INVALID)
       return false;
----------------
Ah, yes, this makes a lot of sense!

================
Comment at: lib/Basic/Targets.cpp:4482
@@ +4481,3 @@
+
+  bool supportsThumb(StringRef CPUAttr) const {
+    return CPUAttr.count('T') || ArchVersion >= 6;
----------------
if you cache CPUAttr, this call loses its argument.

================
Comment at: lib/Basic/Targets.cpp:4486
@@ +4485,3 @@
+
+  bool supportsThumb2(StringRef CPUAttr) const {
+    return CPUAttr.equals("6T2") || ArchVersion >= 7;
----------------
if you cache CPUAttr, this call loses its argument.

================
Comment at: lib/Basic/Targets.cpp:4492
@@ -4504,1 +4491,3 @@
                         MacroBuilder &Builder) const override {
+    StringRef CPUAttr    = getCPUAttr();
+    StringRef CPUProfile = getCPUProfile();
----------------
if you cache CPUAttr, this call disappears.


http://reviews.llvm.org/D10839