Page MenuHomePhabricator

[flang][openmp] Use common Directive and Clause enum from llvm/Frontend
ClosedPublic

Authored by clementval on Jun 30 2020, 1:07 PM.

Details

Summary

This patch is removing the custom enumeration for OpenMP Directives and Clauses and replace them
with the newly tablegen generated one from llvm/Frontend. This is a first patch and some will follow to share the same
infrastructure where possible. The next patch should use the clauses allowance defined in the tablegen file.

Diff Detail

Event Timeline

clementval created this revision.Jun 30 2020, 1:07 PM
clementval edited the summary of this revision. (Show Details)Jun 30 2020, 1:49 PM
clementval added a project: Restricted Project.
DavidTruby accepted this revision.Jun 30 2020, 3:17 PM
DavidTruby added a subscriber: ichoyjx.

This looks really great to me, thanks for all the work you've done here to unify with LLVM! The more code we can share the better.

I've just left one small comment inline that I'd like looked at; otherwise I'm very happy for this to go ahead but I think @ichoyjx and/or @kiranchandramohan should review as well.

As an aside, and I think this is outside the scope of this patch, it'd be great if we could do away with the doAllowedClauses... etc altogether and somehow generate the required information for those from tablegen as well? It seems we should (in most cases) want to share what clauses are allowed on e.g. loop clauses (here called do clauses, in clang called for clauses) where possible too. Is this something you've looked at at all? I'm mostly asking as otherwise I'll take a look but don't want to double up the work!

flang/lib/Semantics/check-omp-structure.cpp
16–66

Is there a reason the constexpr is gone here? The inline was superfluous but there to signal my intent that these should only be used in inline contexts, but the constexpr is actually useful later when we call constexpr functions with these variables.

If it's because the llvm::omp::Clause::* variables aren't themselves constexpr, can they be made so? Their values would seem to me to be known at compile time

This revision is now accepted and ready to land.Jun 30 2020, 3:17 PM
clementval marked 2 inline comments as done.Jun 30 2020, 4:13 PM

This looks really great to me, thanks for all the work you've done here to unify with LLVM! The more code we can share the better.

I've just left one small comment inline that I'd like looked at; otherwise I'm very happy for this to go ahead but I think @ichoyjx and/or @kiranchandramohan should review as well.

Thanks for the quick review and I'll leave some time for other to review.

As an aside, and I think this is outside the scope of this patch, it'd be great if we could do away with the doAllowedClauses... etc altogether and somehow generate the required information for those from tablegen as well? It seems we should (in most cases) want to share what clauses are allowed on e.g. loop clauses (here called do clauses, in clang called for clauses) where possible too. Is this something you've looked at at all? I'm mostly asking as otherwise I'll take a look but don't want to double up the work!

Yeah this is in the pipeline as well. I didn't want to do a too big patch so I have split those two part. Next patch will include the generation of those "allowed/allowedOnce/required" sets for each directives directly from TableGen.

flang/lib/Semantics/check-omp-structure.cpp
16–66

It's because the enumset is now bigger than 64 and the implementation in EnumSet.h switch automatically to std::bitset. This is not possible to use constexpr and initialize a > 64 bitset at compile time. This willl anyway be removed in a follow up patch where it will be generated in TableGen.

DavidTruby marked an inline comment as done.Jun 30 2020, 4:37 PM

Yeah this is in the pipeline as well. I didn't want to do a too big patch so I have split those two part. Next patch will include the generation of those "allowed/allowedOnce/required" sets for each directives directly from TableGen.

This sounds really great, thanks!

flang/lib/Semantics/check-omp-structure.cpp
16–66

Interesting, I didn't know EnumSet had that behaviour. If this is going to be removed in a future patch then I am not bothered about this.
I'm not convinced that these things will be inlined in the way that I would want currently anyway.

ichoyjx accepted this revision.Jul 1 2020, 12:05 PM
ichoyjx marked an inline comment as done.

It took me a while to go through the changes. The merge with tablegen looks very nice!

flang/lib/Semantics/check-omp-structure.cpp
16–66

Nice work on switching to the std::bitset

This revision was automatically updated to reflect the committed changes.
clementval marked an inline comment as done.
ymandel added a subscriber: ymandel.Jul 1 2020, 1:38 PM

Seems like this is causing a slew of compilation warnings, like:

[745/3847] Building CXX object tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaConcept.cpp.o
In file included from /usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Sema/SemaConcept.cpp:24:
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/include/clang/AST/RecursiveASTVisitor.h:2994:14: warning: enumeration values 'OMPC_inbranch', 'OMPC_link', and 'OMPC_notinbranch' not handled in switch [-Wswitch]
  switch (C->getClauseKind()) {
             ^
1 warning generated.
[766/3847] Building CXX object tools/clang/lib/Parse/CMakeFiles/obj.clangParse.dir/ParseOpenMP.cpp.o
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:1689:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:2078:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^

Seems like this is causing a slew of compilation warnings, like:

[745/3847] Building CXX object tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaConcept.cpp.o
In file included from /usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Sema/SemaConcept.cpp:24:
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/include/clang/AST/RecursiveASTVisitor.h:2994:14: warning: enumeration values 'OMPC_inbranch', 'OMPC_link', and 'OMPC_notinbranch' not handled in switch [-Wswitch]
  switch (C->getClauseKind()) {
             ^
1 warning generated.
[766/3847] Building CXX object tools/clang/lib/Parse/CMakeFiles/obj.clangParse.dir/ParseOpenMP.cpp.o
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:1689:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:2078:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^

Yes, sorry for that. I'm working on a fix to handle newly introduce directives/clauses. Should be ok tonight.

MaskRay added a subscriber: MaskRay.EditedJul 1 2020, 2:41 PM

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

llvm/utils/git/pre-push.py can validate the message does not include unneeded tags.

Hi, your git commit contains extra Phabricator tags. You can drop Reviewers: Subscribers: Tags: and the text Summary: from the git commit with the following script:

arcfilter () {
        arc amend
        git log -1 --pretty=%B | awk '/Reviewers:|Subscribers:/{p=1} /Reviewed By:|Differential Revision:/{p=0} !p && !/^Summary:$/ {sub(/^Summary: /,"");print}' | git commit --amend --date=now -F -
}

Reviewed By: is considered important by some people. Please keep the tag. (--date=now is my personal preference (author dates are usually not useful. Using committer dates can make log almost monotonic in time))

llvm/utils/git/pre-push.py can validate the message does not include unneeded tags.

Thanks for the tips. Didn't know some tags were not important and could be dropped. Will use that in the future.

Seems like this is causing a slew of compilation warnings, like:

[745/3847] Building CXX object tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaConcept.cpp.o
In file included from /usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Sema/SemaConcept.cpp:24:
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/include/clang/AST/RecursiveASTVisitor.h:2994:14: warning: enumeration values 'OMPC_inbranch', 'OMPC_link', and 'OMPC_notinbranch' not handled in switch [-Wswitch]
  switch (C->getClauseKind()) {
             ^
1 warning generated.
[766/3847] Building CXX object tools/clang/lib/Parse/CMakeFiles/obj.clangParse.dir/ParseOpenMP.cpp.o
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:1689:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:2078:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^

Yes, sorry for that. I'm working on a fix to handle newly introduce directives/clauses. Should be ok tonight.

If a fix is going to take more than a few minutes (a few hours/the rest of the day is pretty high) could you revert this patch & recommit with the fixes?

Seems like this is causing a slew of compilation warnings, like:

[745/3847] Building CXX object tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaConcept.cpp.o
In file included from /usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Sema/SemaConcept.cpp:24:
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/include/clang/AST/RecursiveASTVisitor.h:2994:14: warning: enumeration values 'OMPC_inbranch', 'OMPC_link', and 'OMPC_notinbranch' not handled in switch [-Wswitch]
  switch (C->getClauseKind()) {
             ^
1 warning generated.
[766/3847] Building CXX object tools/clang/lib/Parse/CMakeFiles/obj.clangParse.dir/ParseOpenMP.cpp.o
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:1689:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:2078:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^

Yes, sorry for that. I'm working on a fix to handle newly introduce directives/clauses. Should be ok tonight.

If a fix is going to take more than a few minutes (a few hours/the rest of the day is pretty high) could you revert this patch & recommit with the fixes?

I reverted it. Will commit back later today.

Seems like this is causing a slew of compilation warnings, like:

[745/3847] Building CXX object tools/clang/lib/Sema/CMakeFiles/obj.clangSema.dir/SemaConcept.cpp.o
In file included from /usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Sema/SemaConcept.cpp:24:
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/include/clang/AST/RecursiveASTVisitor.h:2994:14: warning: enumeration values 'OMPC_inbranch', 'OMPC_link', and 'OMPC_notinbranch' not handled in switch [-Wswitch]
  switch (C->getClauseKind()) {
             ^
1 warning generated.
[766/3847] Building CXX object tools/clang/lib/Parse/CMakeFiles/obj.clangParse.dir/ParseOpenMP.cpp.o
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:1689:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^
/usr/local/google/home/yitzhakm/remote-build/llvm-git/llvm-project/clang/lib/Parse/ParseOpenMP.cpp:2078:11: warning: 19 enumeration values not handled in switch: 'OMPD_distribute_parallel_do', 'OMPD_distribute_parallel_do_simd', 'OMPD_do'
... [-Wswitch]
  switch (DKind) {
          ^

Yes, sorry for that. I'm working on a fix to handle newly introduce directives/clauses. Should be ok tonight.

If a fix is going to take more than a few minutes (a few hours/the rest of the day is pretty high) could you revert this patch & recommit with the fixes?

I reverted it. Will commit back later today.

Thanks!

clementval reopened this revision.Jul 1 2020, 5:34 PM
This revision is now accepted and ready to land.Jul 1 2020, 5:34 PM
clementval updated this revision to Diff 274977.Jul 1 2020, 5:56 PM

Fix for warning

Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2020, 5:56 PM
MaskRay added a comment.EditedJul 1 2020, 6:11 PM

Fix for warning

I find clang better at -Wswitch related warnings than GCC. Might be nice building llvm-project with clang and testing with check-clang.
If you don't mind curl | sh, curl -s https://raw.githubusercontent.com/chromium/chromium/master/tools/clang/scripts/update.py | python - --output-dir=path/to/bin gives you a stable and newer clang+lld+other utilities.

This revision was automatically updated to reflect the committed changes.