Page MenuHomePhabricator

[test-suite] Add test for basic correctness of frame lowering
ClosedPublic

Authored by kristof.beyls on Apr 9 2015, 6:53 AM.

Details

Reviewers
rengolin
echristo
Summary

As part of developing http://reviews.llvm.org/D8876 (adding dynamic stack alignment support
for AArch64), I needed a run-time test to check the basic correctness of frame lowering.
There didn't seem to be a test already that aims to check basic correct generation of
frame setup/prologue code & accesses to objects on the stack.

The added test in this patch uses a template function, so that depending on the
template parameters, the different areas on the stack (arguments, locals, VLAs, alignment
spacing, spill slots, ...) are either present or not present. By instantiating this function
for all combination of its template parameters, the basic correct setup of the frame for
all combinations of frame areas being present or not, is being tested.

I'm afraid the test case needs some C++11 features - so this is the first test added to
the test suite that relies on C++11. I've created a separate sub-directory for tests needing
C++11 features, so hopefully it'll be possible for people to still use the test-suite with
compilers not supporting C++11, if that would be needed.

I think it would be useful to add this to the test-suite, so buildbots running LNT will
detect correctness regressions in frame layout code quickly.

Is adding a test case like this OK? Any review comments on the code?

Thanks,

Kristof

Diff Detail

Event Timeline

kristof.beyls retitled this revision from to [test-suite] Add test for basic correctness of frame lowering.
kristof.beyls updated this object.
kristof.beyls edited the test plan for this revision. (Show Details)
kristof.beyls added subscribers: Unknown Object (MLST), rengolin.
rengolin accepted this revision.Apr 23 2015, 9:32 AM
rengolin added a reviewer: rengolin.

Hi Kristof,

I haven't looked in detail what the whole code does, but I can see how it stress the stack and how the test works, roughly. As this is in a new directory, it'll be easy to disable on targets where it doesn't work yet, or on compilers that don't support.

Apart from my comment, I think this is a good addition to the test-suite.

cheers,
--renato

MultiSource/Makefile
5

This is a bit too generic. I'd add AArch64, maybe ARM, but not enable for everything before making sure all UnitTests work on all platforms we run the test-suite.

This revision is now accepted and ready to land.Apr 23 2015, 9:32 AM
echristo accepted this revision.Apr 23 2015, 11:21 AM
echristo added a reviewer: echristo.
echristo added a subscriber: echristo.

LGTM.

-eric

kristof.beyls added inline comments.Apr 27 2015, 7:09 AM
MultiSource/Makefile
5

The only unit test that this will enable to run extra is the frame_layout unit test that is being added by this patch.
All the other pre-existing unit tests have been moved to a "Mips" directory inside the UnitTests sub-directory.
This way, MultiSource/UnitTests becomes available for non-Mips-specific tests too.

My plan is to commit this patch as is. I expect to potentially see some bots fail on architectures that have e.g. not
fully implemented dynamic stack realignment yet. I'll add skips for those architectures further down in the directory structure,
to just skip the new frame_layout test case. I plan to do the commit of this test in the morning European time, when the commit rate is lower,
to not disrupt others too much if it turns out some architectures do not pass the new frame_layout test.

rengolin added inline comments.Apr 27 2015, 7:28 AM
MultiSource/Makefile
5

Ok, sounds like a plan. Thanks!

kristof.beyls closed this revision.Apr 29 2015, 1:25 PM

I committed this early this morning as r236085