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.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 |
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. |
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. |
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 |
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.
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.
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 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.
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