Details
- Reviewers
grosser Meinersbur bollu
Diff Detail
- Repository
- rPLO Polly
- Build Status
Buildable 16939 Build 16939: arc lint + arc unit
Event Timeline
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 :)
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? |
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).
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.
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.
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. |
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.