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/
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
