[llvm] 781a816 - [llvm][Arm/AArch64] Format extension flags in CPU test failures

[David Spickett via llvm-commits]

Author: David Spickett
Date: 2020-12-22T11:13:36Z
New Revision: 781a816d4cacbd0e73d36b12f82c87c0393b5a5b

URL: https://github.com/llvm/llvm-project/commit/781a816d4cacbd0e73d36b12f82c87c0393b5a5b
DIFF: https://github.com/llvm/llvm-project/commit/781a816d4cacbd0e73d36b12f82c87c0393b5a5b.diff

LOG: [llvm][Arm/AArch64] Format extension flags in CPU test failures

Previously you just two hex numbers you had to decode manually.
This change adds a predicate formatter for extension flags
to produce failure messages like:
```
[ RUN      ] AArch64CPUTests/AArch64CPUTestFixture.testAArch64CPU/2
<...>llvm/unittests/Support/TargetParserTest.cpp:862:
Failure
Expected extension flags: +fp-armv8, +crc, +crypto (0xe)
     Got extension flags: +fp-armv8, +neon, +crc, +crypto (0x1e)
[  FAILED  ] AArch64CPUTests/AArch64CPUTestFixture.testAArch64CPU/2,
where GetParam() = "cortex-a34", "armv8-a", <...>
```

>From there you can take the feature name and map it back
to the enum in ARM/AArch64TargetParser.def.
(which isn't perfect but you've probably got both files
open if you're editing these tests)

Note that AEK_NONE is not meant to be user facing in the compiler
but here it is part of the tests. So failures may show an
extension "none" where the normal target parser wouldn't.

The formatter is implemented as a template on ARM::ISAKind
because the predicate formatters assume all parameters are used
for comparison.
(e.g. PRED_FORMAT3 is for comparing 3 values, not having 3
arguments in general)

Reviewed By: MarkMurrayARM

Differential Revision: https://reviews.llvm.org/D93448

Added: 
    

Modified: 
    llvm/unittests/Support/TargetParserTest.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/unittests/Support/TargetParserTest.cpp b/llvm/unittests/Support/TargetParserTest.cpp
index 5208f6a75b02..bc2fd6243aa5 100644
--- a/llvm/unittests/Support/TargetParserTest.cpp
+++ b/llvm/unittests/Support/TargetParserTest.cpp
@@ -8,7 +8,9 @@
 
 #include "llvm/Support/TargetParser.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/ARMBuildAttributes.h"
+#include "llvm/Support/FormatVariadic.h"
 #include "gtest/gtest.h"
 #include <string>
 
@@ -31,6 +33,47 @@ const char *ARMArch[] = {
     "armv8m.main", "iwmmxt",       "iwmmxt2",     "xscale",       "armv8.1-m.main",
 };
 
+template <ARM::ISAKind ISAKind>
+std::string FormatExtensionFlags(uint64_t Flags) {
+  std::vector<StringRef> Features;
+
+  if (ISAKind == ARM::ISAKind::AARCH64) {
+    // AEK_NONE is not meant to be shown to the user so the target parser
+    // does not recognise it. It is relevant here though.
+    if (Flags & AArch64::AEK_NONE)
+      Features.push_back("none");
+    AArch64::getExtensionFeatures(Flags, Features);
+  } else {
+    if (Flags & ARM::AEK_NONE)
+      Features.push_back("none");
+    ARM::getExtensionFeatures(Flags, Features);
+  }
+
+  // The target parser also includes every extension you don't have.
+  // E.g. if AEK_CRC is not set then it adds "-crc". Not useful here.
+  Features.erase(std::remove_if(Features.begin(), Features.end(),
+                                [](StringRef extension) {
+                                  return extension.startswith("-");
+                                }),
+                 Features.end());
+
+  return llvm::join(Features, ", ");
+}
+
+template <ARM::ISAKind ISAKind>
+testing::AssertionResult
+AssertSameExtensionFlags(const char *m_expr, const char *n_expr,
+                         uint64_t ExpectedFlags, uint64_t GotFlags) {
+  if (ExpectedFlags == GotFlags)
+    return testing::AssertionSuccess();
+
+  return testing::AssertionFailure() << llvm::formatv(
+             "Expected extension flags: {0} ({1:x})\n"
+             "     Got extension flags: {2} ({3:x})\n",
+             FormatExtensionFlags<ISAKind>(ExpectedFlags), ExpectedFlags,
+             FormatExtensionFlags<ISAKind>(GotFlags), GotFlags);
+}
+
 struct ARMCPUTestParams {
   ARMCPUTestParams(StringRef CPUName, StringRef ExpectedArch,
                    StringRef ExpectedFPU, uint64_t ExpectedFlags,
@@ -65,7 +108,8 @@ TEST_P(ARMCPUTestFixture, ARMCPUTests) {
   EXPECT_EQ(params.ExpectedFPU, ARM::getFPUName(FPUKind));
 
   uint64_t default_extensions = ARM::getDefaultExtensions(params.CPUName, AK);
-  EXPECT_EQ(params.ExpectedFlags, default_extensions);
+  EXPECT_PRED_FORMAT2(AssertSameExtensionFlags<ARM::ISAKind::ARM>,
+                      params.ExpectedFlags, default_extensions);
 
   EXPECT_EQ(params.CPUAttr, ARM::getCPUAttr(AK));
 }
@@ -816,7 +860,8 @@ TEST_P(AArch64CPUTestFixture, testAArch64CPU) {
 
   uint64_t default_extensions =
       AArch64::getDefaultExtensions(params.CPUName, AK);
-  EXPECT_EQ(params.ExpectedFlags, default_extensions);
+  EXPECT_PRED_FORMAT2(AssertSameExtensionFlags<ARM::ISAKind::AARCH64>,
+                      params.ExpectedFlags, default_extensions);
 
   unsigned FPUKind = AArch64::getDefaultFPU(params.CPUName, AK);
   EXPECT_EQ(params.ExpectedFPU, ARM::getFPUName(FPUKind));