This is an archive of the discontinued LLVM Phabricator instance.

Add AArch64 unit tests
ClosedPublic

Authored by rovka on Sep 14 2016, 2:06 AM.

Details

Summary

Add unit tests for checking a few tricky instruction sizes. Also remove the old
tests for the instruction sizes, which were clunky and brittle.

Since this is the first set of target-specific unit tests, we need to add some
CMake plumbing. In the future, adding unit tests for a given target will be as
simple as creating a directory with the same name as the target under
unittests/Target. The tests are only run if the target is enabled in
LLVM_TARGETS_TO_BUILD.

Diff Detail

Event Timeline

rovka updated this revision to Diff 71307.Sep 14 2016, 2:06 AM
rovka retitled this revision from to Add AArch64 unit tests.
rovka updated this object.
rovka added a reviewer: ab.
rovka added subscribers: llvm-commits, rengolin.

Hi Diana,

Nice usage of lambdas.

I'm guessing that the MIR snippet you have in runChecks() is the bare minimum to make it parse, in which case, any sequence of machine instructions would work with that pattern. For now, that being inside an anonymous namespace in the AArch64 part of the tests is ok, but if more targets start using this pattern (which I think it's a good thing), we should be moving this up somewhere. Not in this patch, but maybe a comment would be good.

Overall, LGTM, but I'll let someone else have a look, too.

cheers,
--renato

unittests/Target/AArch64/InstSizes.cpp
89

why the extra space?

rovka added a comment.Sep 19 2016, 1:34 AM

Hi. Thanks for looking :)

I'm guessing that the MIR snippet you have in runChecks() is the bare minimum to make it parse, in which case, any sequence of machine instructions would work with that pattern. For now, that being inside an anonymous namespace in the AArch64 part of the tests is ok, but if more targets start using this pattern (which I think it's a good thing), we should be moving this up somewhere. Not in this patch, but maybe a comment would be good.

It's the bare minimum to run these 3 tests, and it's probably good enough for any instruction-size tests. It used to be even smaller before I had to add the global for the TLS test, so it's not impossible for other tests to have other needs as well (e.g. tests that need more than one basic block or more than one function would probably rather not have this particular MIR stub). I think it needs to evolve a bit before being extracted. I'll try to think of a comment along those lines.

unittests/Target/AArch64/InstSizes.cpp
89

Erm, it just felt like a natural way to separate the 2 checks. But I guess with everything else so compact it may look a bit awkward. I'll remove it before committing.

rovka updated this revision to Diff 71793.Sep 19 2016, 3:22 AM

Address Renato's comments.

rovka updated this revision to Diff 73585.Oct 4 2016, 6:08 PM

Rebased. Also ping.

MatzeB accepted this revision.Oct 11 2016, 10:18 AM
MatzeB edited edge metadata.

This test looks good to me.

In general though these programmatic unit tests are harder to understand and more work to maintain than (well done) tests using one of the llvm tool. Of course to do this effectively we need a means to print/test the information/functionality we are interested in and right now there is no way to print the computed size next to each instruction or similar. I am also aware that I did a similar thing in MI/LiveIntervalTest.cpp because there was no good way to force codegen to move exactly 1 instruction to another place, so I guess this is more of a "kids don't do this at home" warning :)

unittests/Target/AArch64/CMakeLists.txt
7–8

Doesn't sound so bad to me that it warrants a "FIXME".

This revision is now accepted and ready to land.Oct 11 2016, 10:18 AM
rovka added a comment.Oct 12 2016, 2:10 AM

In general though these programmatic unit tests are harder to understand and more work to maintain than (well done) tests using one of the llvm tool. Of course to do this effectively we need a means to print/test the information/functionality we are interested in and right now there is no way to print the computed size next to each instruction or similar.

Would it be ok to add a pass that annotates the MIR with the instruction sizes (as comments) just so we can have the usual kind of tests with FileCheck? Just wondering, I'm definitely leaving that for the next person that has to deal with an instruction-size related bug :)

Thanks for reviewing.

This revision was automatically updated to reflect the committed changes.

In general though these programmatic unit tests are harder to understand and more work to maintain than (well done) tests using one of the llvm tool. Of course to do this effectively we need a means to print/test the information/functionality we are interested in and right now there is no way to print the computed size next to each instruction or similar.

Would it be ok to add a pass that annotates the MIR with the instruction sizes (as comments) just so we can have the usual kind of tests with FileCheck? Just wondering, I'm definitely leaving that for the next person that has to deal with an instruction-size related bug :)

Yes I would imagine something in the line of adding a cl::opt flag (restricted to assert builds) that either annotates .mir output or assembly output with comments, or create a new pass that prints the debug information you need (though for this specific case a whole new pass seems a bit much).