[PATCH] D106165: [flang][OpenMP] Add semantic check for target nesting

[Valentin Clement via Phabricator via llvm-commits]

clementval requested changes to this revision.
clementval added inline comments.
This revision now requires changes to proceed.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:291
     }
+    if (GetTARGETNest() > 0) {
+      CheckTARGETNest(x);
----------------
It would be better to have a generic `IsNestedIn` function where you can pass the directive you want to check for. We don't want to have a counter for each construct. 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:516
+  if (eligibleTARGET) {
+    context_.Say(parser::FindSourceLocation(c),
+        "%s directive cannot be nested inside TARGET region"_err_en_US,
----------------
The standard says it is unspecified but not really restricted. 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:665
   PushContextAndClauseSets(beginDir.source, beginDir.v);
+  if (GetContext().directive == llvm::omp::Directive::OMPD_target) {
+    EnterTARGETNest();
----------------
As mentioned above you should use the current `_dirContext` stack with a generic function. 


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:735
 void OmpStructureChecker::Leave(const parser::OpenMPBlockConstruct &) {
+  if (GetContext().directive == llvm::omp::Directive::OMPD_target) {
+    ExitTARGETNest();
----------------
Same here. Please use generic mechanism. 


================
Comment at: flang/test/Semantics/omp-target01.f90:1
+! RUN: %S/test_errors.sh %s %t %flang -fopenmp
+! REQUIRES: shell
----------------
You don't check if your restriction actually work here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106165