This is an archive of the discontinued LLVM Phabricator instance.

[llvm][ARM/AArch64] Convert Target Parser CPU tests to fixtures
ClosedPublic

Authored by DavidSpickett on Dec 16 2020, 4:59 AM.

Details

Summary

Also convert the test function to use EXPECT_EQ and
remove the special case for the AEK_NONE extension.

This means that each test is marked as failing separatley
and the accumultated EXPECT failures are printed next
to that test, with its parameters.

Before they would be hidden by the "pass &=" pattern
and failures would print in one block since it was a
"single" test.

Example of the new failure messages:

ARMCPUTestsPart1/ARMCPUTestFixture.ARMCPUTests/6
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from ARMCPUTestsPart1/ARMCPUTestFixture
[ RUN      ] ARMCPUTestsPart1/ARMCPUTestFixture.ARMCPUTests/6
/work/open_source/nightly-llvm/llvm-project/llvm/unittests/Support/TargetParserTest.cpp:66:
Failure
      Expected: params.ExpectedFlags
      Which is: 3405705229
To be equal to: default_extensions
      Which is: 1
[  FAILED  ] ARMCPUTestsPart1/ARMCPUTestFixture.ARMCPUTests/6, where
GetParam() = "arm8", "armv4", "none", 0xcafef00d, "4" (0 ms)

Diff Detail

Event Timeline

DavidSpickett created this revision.Dec 16 2020, 4:59 AM
DavidSpickett requested review of this revision.Dec 16 2020, 4:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 16 2020, 4:59 AM
DavidSpickett added a reviewer: MarkMurrayARM.

I will add a better expect printer for the extension bitfields in a future change, just reorganising the tests here.

fhahn added a subscriber: fhahn.Dec 16 2020, 5:06 AM
fhahn added inline comments.
llvm/unittests/Support/TargetParserTest.cpp
35–36

Can a struct be used instead here? That would make things much more readable IMO/

  • replace const char* with StringRef in test params
DavidSpickett added inline comments.Dec 16 2020, 5:07 AM
llvm/unittests/Support/TargetParserTest.cpp
35–36

Will do.

Change test params to a structure.

DavidSpickett marked an inline comment as done.Dec 16 2020, 5:17 AM
  • clang format
DavidSpickett added inline comments.Dec 16 2020, 5:19 AM
llvm/unittests/Support/TargetParserTest.cpp
933

This is an example where the CPU has AEK_NONE for its additional extensions and the tests had a special path for that.

I don't think there's much merit to that so I've just added AEK_NONE for those CPUs that don't extend the base arch.

MarkMurrayARM accepted this revision.Dec 16 2020, 6:09 AM

The proposed output contains the exact information I needed with my naive attempt. Also, it does not introduce the technical debt that my hack would have.

This revision is now accepted and ready to land.Dec 16 2020, 6:09 AM
DavidSpickett edited the summary of this revision. (Show Details)Dec 16 2020, 6:33 AM

Added a custom formatter for the params struct.
(the default is to print the bytes of the struct)

DavidSpickett edited the summary of this revision. (Show Details)Dec 16 2020, 8:00 AM

@MarkMurrayARM Just updated to properly format the struct, the example in the summary is updated to what you'll get.

Also posted https://reviews.llvm.org/D93448 for printing the extensions, please take a look.

  • Remove unused includes
  • Remove double AEK_NONE in arm1136jf-s test
This revision was landed with ongoing or failed builds.Dec 22 2020, 1:07 AM
This revision was automatically updated to reflect the committed changes.

I remain content.