This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Change version 4.5 hardcoded clang tests to default OpenMP version
AbandonedPublic

Authored by saiislam on Jul 15 2020, 4:53 AM.

Details

Summary

Many clang tests were hardcoded only for version 4.5. This patch changes
them to run for default version of OpenMP, thus enabling their execution
for recent 5.0 default version.

Diff Detail

Event Timeline

saiislam created this revision.Jul 15 2020, 4:53 AM

[Work in progress]
Among all the test files in clang/test/OpenMP which were running only for version 4.5, I am yet to fix following files (rest all are given in this patch):

  1. clang/test/OpenMP/distribute_parallel_for_codegen.cpp
  2. clang/test/OpenMP/distribute_parallel_for_simd_codegen.cpp
  3. clang/test/OpenMP/distribute_parallel_for_simd_if_messages.cpp
  4. clang/test/OpenMP/for_reduction_messages.cpp
  5. clang/test/OpenMP/parallel_for_simd_if_messages.cpp

Not sure that this is really required. If we want to check for the default version, it would be good to check that it still works for 4.5. It means, need to add additional RUN lines. But I don't think it is profitable to test the same functionality several times without actual effect

Not sure that this is really required. If we want to check for the default version, it would be good to check that it still works for 4.5. It means, need to add additional RUN lines. But I don't think it is profitable to test the same functionality several times without actual effect

These 67 files were testing only for version 4.5. As the default version is now 5.0, isn't it expected that the tests run against it?

Not sure that this is really required. If we want to check for the default version, it would be good to check that it still works for 4.5. It means, need to add additional RUN lines. But I don't think it is profitable to test the same functionality several times without actual effect

These 67 files were testing only for version 4.5. As the default version is now 5.0, isn't it expected that the tests run against it?

It just means that there are no changes in functionality between 4.5 and 5.0. Sure, you can switch to the default version instead but I don't see real profit here. Anyway, it is up to you.

Not sure that this is really required. If we want to check for the default version, it would be good to check that it still works for 4.5. It means, need to add additional RUN lines. But I don't think it is profitable to test the same functionality several times without actual effect

These 67 files were testing only for version 4.5. As the default version is now 5.0, isn't it expected that the tests run against it?

It just means that there are no changes in functionality between 4.5 and 5.0. Sure, you can switch to the default version instead but I don't see real profit here. Anyway, it is up to you.

I was considering a scenario where there is a divergent behavior between 4.5 and a future version X. If we don't remove these version specific strings from the test files which are only testing for 4.5, user won't be notified about the divergent behavior of version X. If these tests fail there, then specific divergent behavior can be annotated with version specific tags, like in most of the other test cases. Does it make sense?

ABataev added inline comments.Jul 15 2020, 6:04 AM
clang/test/OpenMP/parallel_default_messages.cpp
7–8

Same as first 2 lines

clang/test/OpenMP/target_ast_print.cpp
5–11

Better to remove -fopenmp-version from the tests for OpenMP 5.0

saiislam marked an inline comment as done.Jul 15 2020, 6:28 AM
saiislam added inline comments.
clang/test/OpenMP/target_ast_print.cpp
5–11

Yes, that's the plan. D82575 is supposed to remove -fopenmp-version=50 from everywhere. And this patch is supposed to ensure that all tests are testing at least for the default version.

Plan is to keep version strings only at places where there is specific divergent behaviors between different versions.

saiislam abandoned this revision.Aug 3 2020, 12:48 PM

Abandoning in favor of D84844 and D85150 i.e. adding default version testing along with version 4.5 testing.