This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][MLIR][Flang][bbc][Driver] Add fopenmp-version and generate corresponding MLIR attribute
ClosedPublic

Authored by domada on May 11 2023, 3:34 AM.

Details

Summary

This patch adds flag -fopenmp-version to the Flang frontend and bbc tool. This flag is lowered to MLIR OpenMP flag attribute.

Diff Detail

Event Timeline

domada created this revision.May 11 2023, 3:34 AM
Herald added a project: Restricted Project. · View Herald Transcript
domada requested review of this revision.May 11 2023, 3:34 AM
domada updated this revision to Diff 521251.May 11 2023, 4:12 AM

Patch rebased

domada updated this revision to Diff 521282.May 11 2023, 6:03 AM

Patch rebased + clang format

All in all LGTM, but I'm not sure whether Flang should be defaulting to OpenMP 5.0. AFAIK, that's not supported yet.

clang/lib/Driver/ToolChains/Flang.cpp
46

Is this clang-format? Looks like a big block of unrelated changes (not against it).

domada updated this revision to Diff 522157.May 15 2023, 6:25 AM

Patch rebased because I applied remarks for parent revision.

All in all LGTM, but I'm not sure whether Flang should be defaulting to OpenMP 5.0. AFAIK, that's not supported yet.

If you wish I can set to OpenMP 4.5. But then we need to have two separate flags in clang/include/clang/Driver/Options.td (one for clang and the second one for flang).

clang/lib/Driver/ToolChains/Flang.cpp
46

Yes, this is output of clang format.

All in all LGTM, but I'm not sure whether Flang should be defaulting to OpenMP 5.0. AFAIK, that's not supported yet.

If you wish I can set to OpenMP 4.5. But then we need to have two separate flags in clang/include/clang/Driver/Options.td (one for clang and the second one for flang).

We will not be able to match the OpenMP support in clang (stand support level) soon. Although we have made lot of progress, we are effectively around 1.1. To stay true to the meaning of this metadata, we will have to have separate flags. But, I guess, fclang-openmp-version might not be acceptable to Clang folks who are used to using -fopenmp-version. Can't this be achieved by the same flag? On a cursory look, i don't see anything that prevents using the same flag but with different defaults in the code that is handling the flag.

domada updated this revision to Diff 522999.May 17 2023, 4:42 AM

Change the default version of OpenMP version flag to 1.1 for flang. Modify the flag description text. No changes for Clang code.

All in all LGTM, but I'm not sure whether Flang should be defaulting to OpenMP 5.0. AFAIK, that's not supported yet.

If you wish I can set to OpenMP 4.5. But then we need to have two separate flags in clang/include/clang/Driver/Options.td (one for clang and the second one for flang).

We will not be able to match the OpenMP support in clang (stand support level) soon. Although we have made lot of progress, we are effectively around 1.1. To stay true to the meaning of this metadata, we will have to have separate flags. But, I guess, fclang-openmp-version might not be acceptable to Clang folks who are used to using -fopenmp-version. Can't this be achieved by the same flag? On a cursory look, i don't see anything that prevents using the same flag but with different defaults in the code that is handling the flag.

Hi Kiran, you were right. It is possible to set different value in code for the same flag. I updated the patch and I set the default value of OpenMP version to 1.1.

I updated only this patch and I did not change the previous patch: https://reviews.llvm.org/D150351 . Is it ok for you?

LGTM.

Target-related constructs came in as part of OpenMP 4.0. Would you want to have a different default for the device version?

This revision is now accepted and ready to land.May 17 2023, 5:56 AM