[PATCH] D21785: [RFC]Add unittests to {ARM | AArch64}TargetParser

[jojo.ma via llvm-commits]

jojo added inline comments.

================
Comment at: lib/Support/TargetParser.cpp:419
@@ -418,3 +418,3 @@
 #define AARCH64_CPU_NAME(NAME, ID, DEFAULT_FPU, IS_DEFAULT, DEFAULT_EXT) \
-    .Case(NAME, AArch64ARCHNames[ID].ArchBaseExtensions | DEFAULT_EXT)
+    .Case(NAME, DEFAULT_EXT)
 #include "llvm/Support/AArch64TargetParser.def"
----------------
compnerd wrote:
> Isn't this the original bug that is being fixed rather than just adding tests for the target parser?
Yes.this is a bug fixing. For AArch64,it's unnecessary to OR the ArchBaseExtensions,as the tables for ARM and AArch64 have not exactly the same design. 
The original bug is fixed by correcting the incorrect indexing problem and removing the ORing here.All the changes are included in this revision. 

================
Comment at: lib/Support/TargetParser.cpp:468
@@ -466,5 +467,3 @@
 StringRef llvm::AArch64::getArchName(unsigned ArchKind) {
-  for (const auto &AI : AArch64ARCHNames)
-    if (AI.ID == ArchKind)
-      return AI.getName();
-  return StringRef();
+  if (ArchKind >= static_cast<unsigned>(AArch64::ArchKind::AK_LAST))
+    return StringRef();
----------------
compnerd wrote:
> Shouldn't this check for `AK_INVALID` as well?
There is a record for "invalid" in ARCHNames table.
It will return what we want for ArchName and ArchAttr, no need to do additional check.

================
Comment at: lib/Support/TargetParser.cpp:488
@@ -487,5 +487,3 @@
 unsigned llvm::AArch64::getArchAttr(unsigned ArchKind) {
-  for (const auto &AI : AArch64ARCHNames)
-    if (AI.ID == ArchKind)
-      return AI.ArchAttr;
-  return ARMBuildAttrs::CPUArch::v8_A;
+  if (ArchKind >= static_cast<unsigned>(AArch64::ArchKind::AK_LAST))
+    return ARMBuildAttrs::CPUArch::v8_A;
----------------
compnerd wrote:
> Shouldn't this check for `AK_INVALID` as well?
 the same as above

================
Comment at: unittests/Support/TargetParserTest.cpp:180
@@ +179,3 @@
+
+  for (unsigned i = 0; i< array_lengthof(kARMCPUNames); i++)
+    EXPECT_EQ(kARMCPUNames[i].DefaultFPU,
----------------
compnerd wrote:
> space before the <.
> 
> Why not use a range based loop?
> 
>     for (const auto &ARMCPUName : kARMCPUNames)
>       EXPECT_EQ(ARMCPUName.DefaultFPU, ARM::getDefaultFPU(ARMCPUName.Name, 0));
Yes,indeed.
Replace all similar cases.

================
Comment at: unittests/Support/TargetParserTest.cpp:263
@@ +262,3 @@
+    { "xscale", "noxscale", nullptr, nullptr}
+  };
+
----------------
compnerd wrote:
> clang-format this
Do you mean like this?

{ "crc", "+crc"},
... ...
{ "nocrc", "-crc"},
... ...


https://reviews.llvm.org/D21785