This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Update the default version of OpenMP to 5.1
ClosedPublic

Authored by animeshk-amd on Jul 13 2022, 5:23 AM.

Details

Summary

The default version of OpenMP is updated from 5.0 to 5.1 which means if -fopenmp is specified but -fopenmp-version is not specified with clang, the default version of OpenMP is taken to be 5.1. After modifying the Frontend for that, various LIT tests were updated. This patch contains all such changes. At a high level, these are the patterns of changes observed in LIT tests -

  1. RUN lines which mentioned -fopenmp-version=50 need to kept only if the IR for version 5.0 and 5.1 are different. Otherwise only one RUN line with no version info(i.e. default version) needs to be there.
  2. Test cases of this sort already had the RUN lines with respect to the older default version 5.0 and the version 5.1. Only swapping the version specification flag -fopenmp-version from newer version RUN line to older version RUN line is required.
  3. Diagnostics: Remove the 5.0 version specific RUN lines if there was no difference in the Diagnostics messages with respect to the default 5.1.
  4. Diagnostics: In case there was any difference in diagnostics messages between 5.0 and 5.1, mention version specific messages in tests.
  5. If the test contained version specific ifdef's e.g. "#ifdef OMP5" but there were no RUN lines for any other version than 5.X, then bring the code guarded by ifdef's outside and remove the ifdef's.
  6. Some tests had RUN lines for both 5.0 and 5.1 versions, but it is found that the IR for 5.0 is not different from the 5.1, therefore such RUN lines are redundant. So, such duplicated lines are removed.
  7. To generate CHECK lines automatically, use the script llvm/utils/update_cc_test_checks.py

Diff Detail

Event Timeline

animeshk-amd created this revision.Jul 13 2022, 5:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2022, 5:23 AM
animeshk-amd requested review of this revision.Jul 13 2022, 5:23 AM
Herald added a project: Restricted Project. · View Herald Transcript

[OpenMP] lit tests are fixed to suppport the new default version of OpenMP

If -fopenmp is specified but -fopenmp-version is not specified with clang, the default version of OpenMP is taken to be 5.1. In this revision, the lit tests which were not suitable to support the version update or were failing have been fixed.

This revision is now accepted and ready to land.Jul 20 2022, 5:32 AM

[OpenMP] Update remaining tests for OpenMP version update to 5.1

animeshk-amd edited the summary of this revision. (Show Details)Jul 26 2022, 9:04 PM
animeshk-amd edited the summary of this revision. (Show Details)

[OpenMP] Update more LIT tests to support OpenMP version upgrade

Some more LIT tests which were specifying version information in RUN
lines have been modified.

saiislam accepted this revision.Jul 27 2022, 6:53 AM

Thanks, LGTM!

In the multi-company OpenMP meeting, it was decided to defer this update.
So, please don't land this patch yet.

Rebase and fixing the merge conflicts

saiislam accepted this revision.Jun 14 2023, 6:27 AM

Thank you @animeshk-amd!
LGTM!

As discussed in the multi-company OpenMP LLVM meeting, this is the right time to upgrade the default OpenMP spec version to 5.1.

This revision was landed with ongoing or failed builds.Jun 15 2023, 12:25 AM
This revision was automatically updated to reflect the committed changes.

Does https://clang.llvm.org/docs/OpenMPSupport.html need an update? It still says "Clang fully supports OpenMP 4.5" (with many 5.0/5.1 features marked as "worked on" / "unclaimed"), which would make it unusual to put the default on a version that's (according to that status page) only ~30% implemented.

Does https://clang.llvm.org/docs/OpenMPSupport.html need an update? It still says "Clang fully supports OpenMP 4.5" (with many 5.0/5.1 features marked as "worked on" / "unclaimed"), which would make it unusual to put the default on a version that's (according to that status page) only ~30% implemented.

In the multi-company community meeting, the agreement was to move to the 5.1 version assuming that these features are supported.

In the multi-company community meeting, the agreement was to move to the 5.1 version assuming that these features are supported.

We shouldn't need to assume - either the features are supported or not. I thought the status page would be the right place for this information, but perhaps it is out of date? Whoever the openmp stakeholders are here should ensure this information is correct and up-to-date!

I mean, I'm sure the participants in that meeting know the situation much better than I do, but from what's visible from the outside, it looks unusual to default to something that's not yet fully implemented (for all the usual reasons: assuming there are mistakes found in the not-yet-complete implementation of 5.1, you'll then have to break your users to fix it, whereas until this PR, it was an explicit choice of the consumer to use the not-yet-fully-supported 5.1; it's also unusual in the way that a user will get an error for using features that aren't implemented yet, despite being able to see that the default is 5.1)

In the multi-company community meeting, the agreement was to move to the 5.1 version assuming that these features are supported.

We shouldn't need to assume - either the features are supported or not. I thought the status page would be the right place for this information, but perhaps it is out of date? Whoever the openmp stakeholders are here should ensure this information is correct and up-to-date!

I mean, I'm sure the participants in that meeting know the situation much better than I do, but from what's visible from the outside, it looks unusual to default to something that's not yet fully implemented (for all the usual reasons: assuming there are mistakes found in the not-yet-complete implementation of 5.1, you'll then have to break your users to fix it, whereas until this PR, it was an explicit choice of the consumer to use the not-yet-fully-supported 5.1; it's also unusual in the way that a user will get an error for using features that aren't implemented yet, despite being able to see that the default is 5.1)

I understand your concerns. However, I would like to point out that the various stakeholders involved in this community meeting are well aware of the default version update and its consequences as it was discussed well throughout an year. Regarding status page, I agree that the individual contributors for respective features should be responsible for updating the page.

@jdoerfert @RaviNarayanaswamy @ABataev

Are there any features in this table which have been already implemented but have not been tagged?
https://clang.llvm.org/docs/OpenMPSupport.html#openmp-5-1-implementation-details

Are there any features in this table which have been already implemented but have not been tagged?
https://clang.llvm.org/docs/OpenMPSupport.html#openmp-5-1-implementation-details

I ended up opening an issue for this: https://github.com/llvm/llvm-project/issues/63633