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

[Peixin Qiao via Phabricator via llvm-commits]

peixin added a comment.

@kiranchandramohan Thanks for the comments.



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:483
+  // 2.12.5 Target Construct Restriction
+  bool eligibleTarget{false};
+  llvm::omp::Directive directive;
----------------
kiranchandramohan wrote:
> The name of the variable does not match its intention. Like these are the ones that cause a warning if true, right?
Yes. Sorry for that. Reversed the value to match its intention.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:521
+    context_.Say(parser::FindSourceLocation(c),
+        "%s directive should not be nested inside TARGET region"_en_US,
+        parser::ToUpperCaseLetters(getDirectiveName(directive).str()));
----------------
kiranchandramohan wrote:
> Can the warning be changed to what the standard says, that the "behaviour is unspecified"?
Replaced the warning message with what the standard says according to your suggestion.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:291
     }
+    if (GetTARGETNest() > 0) {
+      CheckTARGETNest(x);
----------------
kiranchandramohan wrote:
> peixin wrote:
> > clementval wrote:
> > > 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. 
> > I am thinking about if we can replace this for loop search with the counter marked for `target` nesting since the for loop is executed for each openmp construct check, which is not efficient. With the design in https://reviews.llvm.org/D106335, I only need add one more element in the enum definition of `directiveNestType`. What do you think?
> That sounds good to me.
> 
> I think ideally, you can do the following.
> On visiting a target_data, target_update, target_enter_data or target_exit_data directive check if it is nested in a target region using GetDirectiveNest and report a warning if it is nested.
Thanks for the suggestion. Fixed now.


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

https://reviews.llvm.org/D106165