[PATCH] code refactoring on ARMTargetInfo class

[Renato Golin]

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


REPOSITORY
  rL LLVM

================
Comment at: lib/Basic/Targets.cpp:4097
@@ +4096,3 @@
+    StringRef ArchName = Triple.getArchName();
+    unsigned  ArchISA  = llvm::ARMTargetParser::parseArchISA(ArchName);
+
----------------
These could be cached, too, as members of the class.

================
Comment at: lib/Basic/Targets.cpp:4101
@@ +4100,3 @@
+    ArchProfile = llvm::ARMTargetParser::parseArchProfile(ArchName);
+    ArchVersion = llvm::ARMTargetParser::parseArchVersion(ArchName);
+
----------------
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.

================
Comment at: lib/Basic/Targets.cpp:4338
@@ +4337,3 @@
+    ArchProfile = llvm::ARMTargetParser::parseArchProfile(ArchName);
+    ArchVersion = llvm::ARMTargetParser::parseArchVersion(ArchName);
+ 
----------------
You should also set IsThumb and ShouldUseInlineAtomic

================
Comment at: lib/Basic/Targets.cpp:4489
@@ +4488,3 @@
+                       (ArchVersion == 5 && CPUAttr.count('E')));
+    bool is32Bit = (!IsThumb || supportsThumb2(CPUAttr));
+    if (is5EOrAbove && is32Bit && (CPUProfile != "M" || CPUAttr  == "7EM"))
----------------
This variable name is odd...

http://reviews.llvm.org/D10839

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/