[PATCH] D10839: code refactoring on ARMTargetInfo class

[Renato Golin]

rengolin added inline comments.

================
Comment at: lib/Basic/Targets.cpp:4222
@@ +4221,3 @@
+      setArchInfo(DefaultCPU);
+      ShouldUseInlineAtomic = (ArchISA == llvm::ARM::IK_ARM &&
+                                          ArchVersion >= 6) ||
----------------
labrinea wrote:
> rengolin wrote:
> > ShouldUseInlineAtomic must be set up inside setAtomic(), too.
> 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 "arm**v**" or "thumb**v**".
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.

================
Comment at: lib/Basic/Targets.cpp:4228
@@ +4227,3 @@
+    else {
+      setArchInfo(CPU);
+      ShouldUseInlineAtomic = false;
----------------
labrinea wrote:
> 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.
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.


http://reviews.llvm.org/D10839