This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Prepare unit tests for update to ISL 0.16
ClosedPublic

Authored by Meinersbur on Jan 11 2016, 4:05 PM.

Details

Summary

ISL 0.16 will change how sets are printed which breaks 117 unit tests that text-compare printed sets. This patch re-formats most of these unit tests using a script and small manual editing on top of that. When actually updating ISL, most work is done by just re-running the script to adapt to the changed output.

Some tests that compare IR and tests with single CHECK-lines that can be easily updated manually are not included here.

The re-format script will also be committed afterwards. The per-test formatter invocation command lines options will not be added in the near future because it is ad hoc and would overwrite the manual edits. Ideally it also shouldn't be required anymore because ISL's set printing has become more stable in 0.16.

Diff Detail

Repository
rL LLVM

Event Timeline

Meinersbur updated this revision to Diff 44573.Jan 11 2016, 4:05 PM
Meinersbur retitled this revision from to [Polly] Prepare unit tests for update to ISL 0.16 (WIP).
Meinersbur updated this object.
Meinersbur added reviewers: grosser, jdoerfert.
Meinersbur added a project: Restricted Project.
Meinersbur added subscribers: pollydev, llvm-commits.
grosser edited edge metadata.Jan 11 2016, 10:42 PM

Hi Michael,

the patch looks to me very sensible. I would be generally comfortable to commit something like this. What are the remaining changes you had in mind?

Regarding the script, I think it would be great if the --check-include options could eventually (not necessary for this commit) be auto-derived. Also, does --check-position=autodetect work after this initial formatting? If both would, the majority of the options in https://raw.githubusercontent.com/Meinersbur/polly/update_test/test/CMakeLists.txt would not be necessary any more, right? I think this is would be desirable.

test/ScopInfo/remarks.ll
3 ↗(On Diff #44573)

Why was this line dropped?

the patch looks to me very sensible. I would be generally comfortable to commit something like this. What are the remaining changes you had in mind?

Removing additional lines in reduction_simple_iv_debug_wrapped_dependences.ll and sequential_loops.ll, re-add possibly interesting CHECK-NOT lines that are not covered by CHECK-NEXTs.

Regarding the script, I think it would be great if the --check-include options could eventually (not necessary for this commit) be auto-derived.

Can you share your idea how this autodetection could work?

Also, does --check-position=autodetect work after this initial formatting?

Yes, it looks for the position of the first CHECK, and the reformatted one with --check-position=autodetect will still be in the same position.

If both would, the majority of the options in https://raw.githubusercontent.com/Meinersbur/polly/update_test/test/CMakeLists.txt would not be necessary any more, right?

For the moment I do not plan commit the CMakeLists.txt (appart from the configure_file) in its current form as it is too preliminary, would again destroy any manual edits and from our last phone call discussion it seemed there wouldn't be that much interest.

I think this is would be desirable.

Why?

test/ScopInfo/remarks.ll
3 ↗(On Diff #44573)

Good catch

The RUN line here emits binary data to FileCheck, so I added -analyze (alternatively -S would be possible, but there is another unit test also checking the remark which uses -analyze)

The -analyze output is printed to stdout while the remarks are printed to stderr. How they are combined depends on the receiving program (Terminal, FileCheck, or the python script). For some reason (buffering?) python combines the stdline without its \n, so it writes:

Printing analysis 'Polly - Create polyhedral description of Scops' for region: 'entry.split => if.remark: test/ScopInfo/remarks.c:4:7: SCoP begins here

This is not recognized as a "Remark"-line by the script. Does opt maybe have an option to suppress its output?

the patch looks to me very sensible. I would be generally comfortable to commit something like this. What are the remaining changes you had in mind?

Removing additional lines in reduction_simple_iv_debug_wrapped_dependences.ll and sequential_loops.ll, re-add possibly interesting CHECK-NOT lines that are not covered by CHECK-NEXTs.

Cool.

Regarding the script, I think it would be great if the --check-include options could eventually (not necessary for this commit) be auto-derived.

Can you share your idea how this autodetection could work?

It seems you have only a couple of options Statements, AssumedContext, InvariantAccesses. Most (all) of these options could possibly be identified
by looking for specific string in the CHECK lines. E.g., for AssumedContext.

Also, does --check-position=autodetect work after this initial formatting?

Yes, it looks for the position of the first CHECK, and the reformatted one with --check-position=autodetect will still be in the same position.

Nice.

If both would, the majority of the options in https://raw.githubusercontent.com/Meinersbur/polly/update_test/test/CMakeLists.txt would not be necessary any more, right?

For the moment I do not plan commit the CMakeLists.txt (appart from the configure_file) in its current form as it is too preliminary, would again destroy any manual edits and from our last phone call discussion it seemed there wouldn't be that much interest.

Sure.

I think this is would be desirable.

Why?

If I would like to update a test case using your script, it's nicer if it just works rather than me having to figure out all options.

Best,
Tobias

Regarding the script, I think it would be great if the --check-include options could eventually (not necessary for this commit) be auto-derived.

Can you share your idea how this autodetection could work?

It seems you have only a couple of options Statements, AssumedContext, InvariantAccesses. Most (all) of these options could possibly be identified
by looking for specific string in the CHECK lines. E.g., for AssumedContext.

This is extremely fragile. Just imagine we change the -analyze output. The script would need to recognize the old and new format.
And because we are not upgrading all tests at once, all the formats of the past, including degenerated ones. Also, this would not work with new test cases. You'd need to first write a correct test case manually before one can use the script.

I think this is would be desirable.

Why?

If I would like to update a test case using your script, it's nicer if it just works rather than me having to figure out all options.

We might also standardize unit tests. That is, tests in DependenceInfo have all dependences checked, ScoopInfo the Statements, etc.

Meinersbur updated this revision to Diff 44640.Jan 12 2016, 9:03 AM
Meinersbur edited edge metadata.
Meinersbur marked 2 inline comments as done.

Solved problem of interleaving stderr/stdout by disabling opt's stdout output and testing it with its own RUN line

Meinersbur updated this revision to Diff 44815.Jan 13 2016, 4:56 PM
Meinersbur retitled this revision from [Polly] Prepare unit tests for update to ISL 0.16 (WIP) to [Polly] Prepare unit tests for update to ISL 0.16.
Meinersbur updated this object.

Manual edits (+some minor script changes)

grosser accepted this revision.Jan 13 2016, 9:27 PM
grosser edited edge metadata.

lgtm

This revision is now accepted and ready to land.Jan 13 2016, 9:27 PM
This revision was automatically updated to reflect the committed changes.