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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.
This revision now requires review to proceed.

LGTM. One minor comment.

@clementval do you have further comments?



================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:481
+  // 2.12.5 Target Construct Restriction
+  bool eligibleTarget{true};
+  llvm::omp::Directive directive;
----------------
peixin wrote:
> kiranchandramohan wrote:
> > Nit:Would eligibleTargetDir be better?
> elibleTargetDir does not match its intention. Would ineligibleTargetDir be better?
Yes, I would prefer ineligibleTargetDir.


================
Comment at: flang/lib/Semantics/check-omp-structure.cpp:291
     }
+    if (GetTARGETNest() > 0) {
+      CheckTARGETNest(x);
----------------
peixin wrote:
> kiranchandramohan wrote:
> > peixin wrote:
> > > 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.
> > I was suggesting something like the following to avoid double traversal of OpenMPBlockConstruct. We would be visiting these nodes anyway during regular traversal.
> > Have you considered this? Feel free to dissent.
> > ```
> > void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
> >   const auto &beginBlockDir{std::get<parser::OmpBeginBlockDirective>(c.t)};
> >   const auto &beginDir{std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
> >    if (GetDirectiveNest(TargetNest) > 0) {
> >      if (beginDir.v == llvm::omp::Directive::OMPD_target_data) {
> >        //print warning
> >      }
> > }
> > ```
> Actually, my first implementation is like what you suggest. Arnamoy suggests the current implementation and I argee with him. Merging the semantic checks for `target_data` and other three directives in one function seems easier to read and understand. In addition, implementing the semantic check of the four directives nested inside `target` in one fuction seems more reasonable since they are in one restriction. This is one time-readability tradeoff. I think the double traversal does not take too much time. What do you think?
OK.


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

https://reviews.llvm.org/D106165