This is an archive of the discontinued LLVM Phabricator instance.

[Polly][NewPM] Port IslAst, ScopDetect, and ScopInfo lit testsuites over to the new PM
AcceptedPublic

Authored by philip.pfaffe on Apr 10 2018, 8:36 AM.

Details

Summary

These changes are entirely mechanic, using the new-pm pipeline specifications.
I didn't do these changes by hand but instead used a small
tool, with a small bit of
manual polishing.

This patch depends on D45484.

Event Timeline

philip.pfaffe created this revision.Apr 10 2018, 8:36 AM

It's obviously hard for reviewers to examine all changes, but I'd be happy if you could take a look at a random sample :)

grosser accepted this revision.Apr 10 2018, 9:20 AM

Very nice. This LGTM, but maybe @Meinersbur also has some comment.

This revision is now accepted and ready to land.Apr 10 2018, 9:20 AM

Instead of adding -aa-pipeline=basic-aa everywhere, did you consider adding it to %loadPolly, or only add it if the test otherwise fails?

This will double the time a check-polly run takes, which I unfortunately cannot get too excited about. For new test cases, do we then also have to consider the new pass manager?
My suggestion is to wait with this change until we want to make the new PM the main implementation, instead of maintaining two PMs in parallel. The script can be applied anytime, and even added to the repository.

test/Isl/CodeGen/20100708.ll
1

Could you try to do something more intelligent than merging stdout and stderr everywhere? How they are interleaved is undefined, and we risk indeterministic test failures.

test/Isl/CodeGen/conflict-between-loop-invariant-code-hosting-and-escape-map-computation.ll
1

Could you can tell your script to remove -polly-process-unprofitable, which is added by %loadPolly anyway?

Instead of adding -aa-pipeline=basic-aa everywhere, did you consider adding it to %loadPolly, or only add it if the test otherwise fails?

I can add it to %loadPolly. I'm hesitant to make this opt-in, though, since I fear the tests could get very flaky.

This will double the time a check-polly run takes, which I unfortunately cannot get too excited about. For new test cases, do we then also have to consider the new pass manager?

New test cases should be written for the new pass manager. Possibly, we should require future changes to be new-pm only? I don't think it makes sense to invest in a legacy codebase.

My suggestion is to wait with this change until we want to make the new PM the main implementation, instead of maintaining two PMs in parallel. The script can be applied anytime, and even added to the repository.

I can agree with that, but I think we should make the switch asap (since this is mostly a change in policy).

Instead of adding -aa-pipeline=basic-aa everywhere, did you consider adding it to %loadPolly, or only add it if the test otherwise fails?

I can add it to %loadPolly. I'm hesitant to make this opt-in, though, since I fear the tests could get very flaky.

Can you elaborate on this?

This will double the time a check-polly run takes, which I unfortunately cannot get too excited about. For new test cases, do we then also have to consider the new pass manager?

New test cases should be written for the new pass manager. Possibly, we should require future changes to be new-pm only? I don't think it makes sense to invest in a legacy codebase.

My suggestion is to wait with this change until we want to make the new PM the main implementation, instead of maintaining two PMs in parallel. The script can be applied anytime, and even added to the repository.

I can agree with that, but I think we should make the switch asap (since this is mostly a change in policy).

I'd go with when clang/LLVM itself makes the switch.

Instead of adding -aa-pipeline=basic-aa everywhere, did you consider adding it to %loadPolly, or only add it if the test otherwise fails?

I can add it to %loadPolly. I'm hesitant to make this opt-in, though, since I fear the tests could get very flaky.

Can you elaborate on this?

In a legacy PM pipeline, basic-aa is always the default. I.e., if you do AA queries without specifying any AA passes, you always get basic-aa results. In the new PM, that's not true. So i think it should be present in the testcases. I'm happy to add it to `%loadPolly', though!

This will double the time a check-polly run takes, which I unfortunately cannot get too excited about. For new test cases, do we then also have to consider the new pass manager?

New test cases should be written for the new pass manager. Possibly, we should require future changes to be new-pm only? I don't think it makes sense to invest in a legacy codebase.

My suggestion is to wait with this change until we want to make the new PM the main implementation, instead of maintaining two PMs in parallel. The script can be applied anytime, and even added to the repository.

I can agree with that, but I think we should make the switch asap (since this is mostly a change in policy).

I'd go with when clang/LLVM itself makes the switch.

I'd prefer to have coverage before that. But actually this feels like we should have broader discussion on whether or when we should deprecate legacy PM in Polly and should expect contributors to code against the new PM instead.

lksbhm added a subscriber: lksbhm.Apr 16 2018, 4:58 AM
lksbhm added inline comments.
test/ScopInfo/scalar_to_array.ll
5

This is the same RUN line twice. I just noticed this by accident. The polly-scops and polly-function-scops flags map to the same new-pm pass. It might be good to look for similar cases within the other tests.

In a legacy PM pipeline, basic-aa is always the default. I.e., if you do AA queries without specifying any AA passes, you always get basic-aa results. In the new PM, that's not true. So i think it should be present in the testcases. I'm happy to add it to `%loadPolly', though!

I'd prefer to have it added to %loadPolly to avoid boilerplate in every test case.