Page MenuHomePhabricator

[test-suite] Only add SOLLVE V&V tests expected to work with Clang.
ClosedPublic

Authored by Meinersbur on Sep 19 2021, 5:56 PM.

Details

Summary

Some of the test-suite's tests fail with top-of-tree Clang/libomp. Features might not have matured or not supported at all yet (e.g. #pragma omp loop), but it is also possible that tests are not correct according to the specification.

Regardless the reason, this patch isolates the tests that are currently working helping ensuring that those tests are not regressing. All other tests are skipped. The upstream SOLLVE V&V test may add additional tests in the future and Clang add support for existing tests, in which case the list needs to be updated.

Diff Detail

Repository
rT test-suite

Event Timeline

Meinersbur created this revision.Sep 19 2021, 5:56 PM
Meinersbur requested review of this revision.Sep 19 2021, 5:56 PM

Nice addition. Looking forward to seeing it in your Staging builder.
Also would like to try enabling same at some point in the future for one of the two amdgpu builders.

will this patch get your builder to green status ?

Thanks for adding this Michael.

I saw you are adding the tests that are expected to pass in the cmake but made changes to allow expected fails in LIT. Wouldn’t it make sense to have expected failures in cmake as well. It seems more intuitive to me to mention what is not expected to work. If new tests are added and they pass then that’s fine. It’s more those that are added that do not pass that should trigger a warning.

Other than that it looks good.

Thanks for adding this Michael.

I saw you are adding the tests that are expected to pass in the cmake but made changes to allow expected fails in LIT. Wouldn’t it make sense to have expected failures in cmake as well. It seems more intuitive to me to mention what is not expected to work. If new tests are added and they pass then that’s fine. It’s more those that are added that do not pass that should trigger a warning.

Other than that it looks good.

Never mind. I missed the declarations of all the states. I am still worried if having separate lists will make this hard to maintain. What if this list is added from a file?

will this patch get your builder to green status ?

No, llvm.org/PR50739, llvm.org/PR49940 and llvm.org/PR51233 are still open.

I am still worried if having separate lists will make this hard to maintain. What if this list is added from a file?

I hope we will discussing this in the next OpenMP-in-LLVM call. Upstream changes from SOLLVE V&V could cause the build to fail. That could be sollved (*pun*) by fixing the git sha1 for sollve_vv. It may still be surprising that when fixing a bug in Clang/libomp, the test-suite starts to fail. We may either use --allow-fail instead of --expect-fail.

However, all that might still be more complicated than it needs to be. Simplest would be to have only one list of supported test and skip everything else. No change to lit/timit required. However, the list of the current state might be useful for the moment.

Meinersbur updated this revision to Diff 377268.Oct 5 2021, 8:49 AM

The result of the discussion was to ignore everything that is not known to work yet, including new tests introduced by the upstream project. Updating the diff accordingly.

The update removes the tooling that allowed expected failures while compiling, linking, or executing. Test not known to pass are just skipped.

Meinersbur retitled this revision from [test-suite] Categorize SOLLVE V&V by Clang support. to [test-suite] Only add SOLLVE V&V tests expected to work with Clang..Oct 5 2021, 8:53 AM
Meinersbur edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Oct 21 2021, 7:37 PM