[PATCH] D83326: [flang][openmp] Check clauses allowed semantic with tablegen generated map

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:67
-    llvm::omp::Clause::OMPC_reduction};
-
 std::string OmpStructureChecker::ContextDirectiveAsFortran() {
----------------
I love these patches :) And we clean up Clang after!


================
Comment at: flang/test/Semantics/omp-clause-validity01.f90:457
 
-  !ERROR: REDUCTION clause is not allowed on the TASKLOOP SIMD directive
   !$omp taskloop simd reduction(+:a)
----------------
clementval wrote:
> ichoyjx wrote:
> > clementval wrote:
> > > As a side note, This is supposed to be fine in Clang so I removed the check. I looked at the OpenMP 5.0 std and didn't see a restriction on `reduction` for `task loop simd`.
> > What's the current plan? Are we trying to cover OpenMP 5.0 Spec for semantics (it appears so)?
> Clang just moved to 5.0as default and my guesses that we are targeting 5.0 as well since it is the current standard. 
If Flang has the equivalent to `-fopenmp-version=XX` or you just hardcode a version to be used, it should be possible to retain this error for the 4.5 case, right? 


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMP.td:216
+    VersionedClause<OMPC_ProcBind>,
+  ];
 }
----------------
Yay! :)


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMP.td:229
     VersionedClause<OMPC_Allocate>,
     VersionedClause<OMPC_Detach, 50>,
     VersionedClause<OMPC_Affinity, 50>
----------------
clementval wrote:
> ichoyjx wrote:
> > Bear with me, what does 50 mean?
> The `VersionedClause` is defined as this: `VersionedClause<VersionedClause<Clause c, int min = 1, int max = 0x7FFFFFFF>`
> 
> So here it means the clause is valid from version 5.0 and up. This is currently not used in Flang but in Clang it's used and I took the value from the old macros definition `OMPKinds.def`. So version 4.5 would 45 and so on. 
FWIW, that is how we specify the version on the command line (for clang) as well.



================
Comment at: llvm/utils/TableGen/DirectiveEmitter.cpp:305
 
-  const auto &Directives = Records.getAllDerivedDefinitions("Directive");
-  const auto &Clauses = Records.getAllDerivedDefinitions("Clause");
+  IfDefScope Scope("GEN_FLANG_DIRECTIVE_CLAUSE_SETS", OS);
+
----------------
Any reason this is flang specific? I guess we want to use the information in Clang too (soonish).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83326