Page MenuHomePhabricator

[Attributor] Unify testing (=updates,prefixes,run configurations,...)
ClosedPublic

Authored by jdoerfert on Mar 22 2020, 11:31 PM.

Details

Summary

When the Attributor was created the test update scripts were not well
suited to deal with the challenges of IR attribute checking. This
partially improved.

Since then we also added three additional configurations that need
testing; in total we now have the following four:
{ TUNIT, CGSCC } x { old pass manager (OPM), new pass manager (NPM) }

Finally, the number of developers and tests grew rapidly (partially due
to the addition of ArgumentPromotion and IPConstantProp tests), which
resulted in tests only being run in some configurations, different
prefixes being used, and different "styles" of checks being used.

Due to the above reasons I believed we needed to take another look at
the test update scripts. While we started to use them, via UTC_ARGS:
--enable/disable, the other problems remained. To improve the testing
situation for *all* configurations, to simplify future updates to the
test, and to help identify subtle effects of future changes, we now use
the test update scripts for (almost) all Attributor tests.

An exhaustive prefix list minimizes the number of check lines and makes
it easy to identify and compare configurations.

Tests have been adjusted in the process but we tried to keep their
intend unchanged.

Diff Detail

Event Timeline

jdoerfert created this revision.Mar 22 2020, 11:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2020, 11:31 PM

It is a good idea to generalise the way we write tests.

we now use the test update scripts for (almost) all Attributor tests.

What is the problem with those that are not included in this patch? I see noreturn_async/sync are not here

It is a good idea to generalise the way we write tests.

we now use the test update scripts for (almost) all Attributor tests.

What is the problem with those that are not included in this patch? I see noreturn_async/sync are not here

The script cannot handle the personality syntax just yet. We can still update the entire folder:
llvm/utils/update_test_checks.py --update-only llvm/test/Transforms/Attributor/**/*.ll

uenoku added inline comments.Mar 25 2020, 5:31 AM
llvm/test/Transforms/Attributor/ArgumentPromotion/pr3085.ll
0

Is this test change reasonable?

jdoerfert marked an inline comment as done.Mar 25 2020, 7:42 AM
jdoerfert added inline comments.
llvm/test/Transforms/Attributor/ArgumentPromotion/pr3085.ll
0

Good catch. Probably not. I'll disable check line generation for this function.

any other feedback on this?

sstefan1 accepted this revision.Mar 31 2020, 5:02 AM

LGTM! sorry for the delay

This revision is now accepted and ready to land.Mar 31 2020, 5:02 AM
This revision was automatically updated to reflect the committed changes.

I will disable the run lines in the three Attributor tests for now. However, I was unable to reproduce this even with expensive checks enabled. I think it is a windows only issue. If you are able to provide me somehow with more information, e.g., a valgrind or sanitizer run of the opt calls, that would help me a lot.

@gkistanova also reported this.