[PATCH] D26343: [RFC] Refactor TargetParserTests

[Renato Golin via llvm-commits]

rengolin added a comment.

Thanks Jojo,

It's looking much better. A new round of comments.

cheers,
--renato



================
Comment at: unittests/Support/TargetParserTest.cpp:32
 
-template <typename T, size_t N>
-bool contains(const T (&array)[N], const T element) {
-  return std::find(std::begin(array), std::end(array), element) !=
-         std::end(array);
-}
-
-template <size_t N>
-bool contains(const char *(&array)[N], const char *element) {
-  return std::find_if(std::begin(array), std::end(array), [&](const char *S) {
-           return ::strcmp(S, element) == 0;
-         }) != std::end(array);
-}
-
-TEST(TargetParserTest, ARMArchName) {
-  for (ARM::ArchKind AK = static_cast<ARM::ArchKind>(0);
-       AK <= ARM::ArchKind::AK_LAST;
-       AK = static_cast<ARM::ArchKind>(static_cast<unsigned>(AK) + 1))
-    EXPECT_TRUE(AK == ARM::AK_LAST ? ARM::getArchName(AK).empty()
-                                   : !ARM::getArchName(AK).empty());
-}
-
-TEST(TargetParserTest, ARMCPUAttr) {
-  for (ARM::ArchKind AK = static_cast<ARM::ArchKind>(0);
-       AK <= ARM::ArchKind::AK_LAST;
-       AK = static_cast<ARM::ArchKind>(static_cast<unsigned>(AK) + 1))
-    EXPECT_TRUE((AK == ARM::AK_INVALID || AK == ARM::AK_LAST)
-                    ? ARM::getCPUAttr(AK).empty()
-                    : !ARM::getCPUAttr(AK).empty());
-}
-
-TEST(TargetParserTest, ARMSubArch) {
-  for (ARM::ArchKind AK = static_cast<ARM::ArchKind>(0);
-       AK <= ARM::ArchKind::AK_LAST;
-       AK = static_cast<ARM::ArchKind>(static_cast<unsigned>(AK) + 1))
-    EXPECT_TRUE((AK == ARM::AK_INVALID || AK == ARM::AK_IWMMXT ||
-                 AK == ARM::AK_IWMMXT2 || AK == ARM::AK_LAST)
-                    ? ARM::getSubArch(AK).empty()
-                    : !ARM::getSubArch(AK).empty());
-}
+bool testARMCPU(StringRef cpu, StringRef ExpectedArch, StringRef ExpectedFPU,
+                StringRef ExpectedFlags, StringRef CPUAttr) {
----------------
"cpu" -> "CPUName"


================
Comment at: unittests/Support/TargetParserTest.cpp:42
+  unsigned ExtKind = ARM::getDefaultExtensions(cpu, ArchKind);
+  if (ExtKind <= 1)
+    ret &= ARM::getArchExtName(ExtKind).equals(ExpectedFlags);
----------------
nit: curly brackets to help identifying the if with the else.


================
Comment at: unittests/Support/TargetParserTest.cpp:43
+  if (ExtKind <= 1)
+    ret &= ARM::getArchExtName(ExtKind).equals(ExpectedFlags);
+  else {
----------------
I think this could also work if any one flag was set by itself, so:

    if (ExtKind <= 1 || isPowerOfTwo(ExtKind)) {

could be a more optimal solution, to avoid the long path.


================
Comment at: unittests/Support/TargetParserTest.cpp:45
+  else {
+    unsigned BaseExt = ARM::AEK_CRC | ARM::AEK_CRYPTO | ARM::AEK_DSP |
+                       ARM::AEK_HWDIVARM | ARM::AEK_HWDIV | ARM::AEK_MP |
----------------
I have the impression that the string representation is redundant.

Instead of generating the string representation of the default extensions, then comparing to the given string, I think you should just pass the values as unsigned ex. (CRC | DSP), then compare with the result from `getDefaultExtensions`.


================
Comment at: unittests/Support/TargetParserTest.cpp:189
+TEST(TargetParserTest, testARMArch) {
+  EXPECT_TRUE(
+      testARMArch("armv2", "arm2", "v2", ARMBuildAttrs::CPUArch::Pre_v4));
----------------
formatting: move all lines to the same style.

Pass clang-format on the patch, and then make sure that the alignment is the same, even if not needed.

Example, if one call needs the ARMBuildAttrs to be on a new line (>80 cols), then all of them should.

This makes it easier to "see" what's going on.


================
Comment at: unittests/Support/TargetParserTest.cpp:253
+
+TEST(TargetParserTest, testARMExtension) {
+  EXPECT_FALSE(testARMExtension("arm2", 0, "crc"));
----------------
I like these tests, but would be good to make them check for a feature that is close enough, but not implemented.

For example, vfp on pre-v5, vfpv3 on pre-v7, vfpv4 on cortex-a8, etc.

Also, the v7M cores are redundant, you only need to test one v6M (M0) and one v7M (M3).


================
Comment at: unittests/Support/TargetParserTest.cpp:468
 
-TEST(TargetParserTest, AArch64DefaultFPU) {
-  for (unsigned AK = 0; AK < static_cast<unsigned>(AArch64::ArchKind::AK_LAST);
-       AK++)
-    EXPECT_EQ(kAArch64ARCHNames[AK].DefaultFPU,
-              AArch64::getDefaultFPU(StringRef("generic"), AK));
-
-  for (const auto &AArch64CPUName : kAArch64CPUNames)
-    EXPECT_EQ(AArch64CPUName.DefaultFPU,
-              AArch64::getDefaultFPU(AArch64CPUName.Name,
-                                     static_cast<unsigned>(AArch64CPUName.ID)));
-}
-
-TEST(TargetParserTest, AArch64DefaultExt) {
-  for (unsigned AK = 0; AK < static_cast<unsigned>(AArch64::ArchKind::AK_LAST);
-       AK++)
-    EXPECT_EQ(kAArch64ARCHNames[AK].ArchBaseExtensions,
-              AArch64::getDefaultExtensions(StringRef("generic"), AK));
-
-  for (const auto &AArch64CPUName : kAArch64CPUNames)
-    EXPECT_EQ(
-        AArch64CPUName.DefaultExt,
-        AArch64::getDefaultExtensions(
-            AArch64CPUName.Name, static_cast<unsigned>(AArch64CPUName.ID)));
+bool testAArch64CPU(StringRef cpu, StringRef ExpectedArch,
+                    StringRef ExpectedFPU, StringRef ExpectedFlags,
----------------
The same comments for the ARM implementation apply here.


================
Comment at: unittests/Support/TargetParserTest.cpp:541
+                              ARMBuildAttrs::CPUArch::v8_A));
+  EXPECT_TRUE(testAArch64Arch("armv8.2-a", "generic", "v8.2a",
+                              ARMBuildAttrs::CPUArch::v8_A));
----------------
We now have v8M and v8R.


================
Comment at: unittests/Support/TargetParserTest.cpp:563
+  EXPECT_FALSE(testAArch64Extension("generic", ARM::AK_ARMV8_1A, "ras"));
+  EXPECT_FALSE(testAArch64Extension("generic", ARM::AK_ARMV8_2A, "fp"));
 }
----------------
I think v8.2A has FP by default. If this test returns FALSE, then there's something wrong with the code. :)


https://reviews.llvm.org/D26343