[PATCH] D82906: [flang][openmp] Use common Directive and Clause enum from llvm/Frontend

[Valentin Clement via Phabricator via llvm-commits]

clementval marked 2 inline comments as done.
clementval added a comment.

In D82906#2124096 <https://reviews.llvm.org/D82906#2124096>, @DavidTruby wrote:

> 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.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:16
 
-static constexpr inline OmpClauseSet doAllowedClauses{OmpClause::PRIVATE,
-    OmpClause::FIRSTPRIVATE, OmpClause::LASTPRIVATE, OmpClause::LINEAR,
-    OmpClause::REDUCTION};
-static constexpr inline OmpClauseSet doAllowedOnceClauses{
-    OmpClause::SCHEDULE, OmpClause::COLLAPSE, OmpClause::ORDERED};
-
-static constexpr inline OmpClauseSet simdAllowedClauses{OmpClause::LINEAR,
-    OmpClause::ALIGNED, OmpClause::PRIVATE, OmpClause::LASTPRIVATE,
-    OmpClause::REDUCTION};
-static constexpr inline OmpClauseSet simdAllowedOnceClauses{
-    OmpClause::COLLAPSE, OmpClause::SAFELEN, OmpClause::SIMDLEN};
-
-static constexpr inline OmpClauseSet parallelAllowedClauses{OmpClause::DEFAULT,
-    OmpClause::PRIVATE, OmpClause::FIRSTPRIVATE, OmpClause::SHARED,
-    OmpClause::COPYIN, OmpClause::REDUCTION};
-static constexpr inline OmpClauseSet parallelAllowedOnceClauses{
-    OmpClause::IF, OmpClause::NUM_THREADS, OmpClause::PROC_BIND};
-
-static constexpr inline OmpClauseSet taskloopAllowedClauses{OmpClause::SHARED,
-    OmpClause::PRIVATE, OmpClause::FIRSTPRIVATE, OmpClause::LASTPRIVATE,
-    OmpClause::DEFAULT, OmpClause::UNTIED, OmpClause::MERGEABLE,
-    OmpClause::NOGROUP};
-static constexpr inline OmpClauseSet taskloopAllowedOnceClauses{
-    OmpClause::COLLAPSE, OmpClause::IF, OmpClause::FINAL, OmpClause::PRIORITY};
-static constexpr inline OmpClauseSet taskloopAllowedExclusiveClauses{
-    OmpClause::GRAINSIZE, OmpClause::NUM_TASKS};
-
-static constexpr inline OmpClauseSet distributeAllowedClauses{
-    OmpClause::PRIVATE, OmpClause::FIRSTPRIVATE, OmpClause::LASTPRIVATE};
-static constexpr inline OmpClauseSet distributeAllowedOnceClauses{
-    OmpClause::COLLAPSE, OmpClause::DIST_SCHEDULE};
-
-static constexpr inline OmpClauseSet targetAllowedClauses{OmpClause::IF,
-    OmpClause::PRIVATE, OmpClause::FIRSTPRIVATE, OmpClause::MAP,
-    OmpClause::IS_DEVICE_PTR, OmpClause::DEPEND};
-static constexpr inline OmpClauseSet targetAllowedOnceClauses{
-    OmpClause::DEVICE, OmpClause::DEFAULTMAP, OmpClause::NOWAIT};
-
-static constexpr inline OmpClauseSet teamsAllowedClauses{OmpClause::PRIVATE,
-    OmpClause::FIRSTPRIVATE, OmpClause::SHARED, OmpClause::REDUCTION};
-static constexpr inline OmpClauseSet teamsAllowedOnceClauses{
-    OmpClause::NUM_TEAMS, OmpClause::THREAD_LIMIT, OmpClause::DEFAULT};
-
-static constexpr inline OmpClauseSet sectionsAllowedClauses{OmpClause::PRIVATE,
-    OmpClause::FIRSTPRIVATE, OmpClause::LASTPRIVATE, OmpClause::REDUCTION};
+static OmpClauseSet doAllowedClauses{llvm::omp::Clause::OMPC_private,
+    llvm::omp::Clause::OMPC_firstprivate, llvm::omp::Clause::OMPC_lastprivate,
----------------
DavidTruby wrote:
> 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
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82906/new/

https://reviews.llvm.org/D82906