[PATCH] D92735: [Flang] [OpenMP] Add semantic checks for invalid branch into/from OpenMP constructs

[Praveen G via Phabricator via llvm-commits]

praveen marked 5 inline comments as done.
praveen added a comment.

@kiranchandramohan I guess the checks work for all the OpenMP constructs . I have added the code for omp critical constructs to identify the invalid entry and branch to/from errors.

@sameeranjoshi I have made the changes to make use of the logic in resolve-directives.cpp instead of the 'LabelEnforce' class. Please let us know if you have any other suggestions regarding this . Thanks for the changes made to make use of LabelEnforce.



================
Comment at: flang/lib/Semantics/resolve-directives.cpp:240
+      }
+      targetLabels_.emplace(label, thisContext);
+      auto range{sourceLabels_.equal_range(label)};
----------------
kiranchandramohan wrote:
> Add a few comments on what is done on seeing a statement with a label.
Done


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:345
 
+  void Post(const parser::GotoStmt &gotoStmt) { CheckSourceLabel(gotoStmt.v); }
+  void Post(const parser::ComputedGotoStmt &computedGotoStmt) {
----------------
kiranchandramohan wrote:
> Add a few comments on what is done on seeing a statement which can cause a jump to a label.
Done


================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1504
+    if (!sourceContext) {
+      context_.Say(
+          source, "invalid entry to OpenMP structured block"_err_en_US);
----------------
kiranchandramohan wrote:
> Can the error message also contain the entry point into the OpenMP region? CheckBranchesIntoDoBody prints error messages like that
Yes . Modified the error messages to contain the entry point into the OpenMP region


================
Comment at: flang/test/Semantics/omp-invalid-branch.f90:11
+  !$omp parallel
+  !ERROR: Control flow escapes from PARALLEL
+  open (10, file="myfile.dat", err=100)
----------------
kiranchandramohan wrote:
> praveen wrote:
> > kiranchandramohan wrote:
> > > Why doesn't the other error (invalid branches to/from OpenMP structured blocks) show here?
> > @kiranchandramohan The error "invalid branches to/from OpenMP structured blocks" will be identified in the CheckLabelContext function for the cases where the control flow reaches out of OpenMP structured blocks. 
> > 
> > Because //we are already throwing// the "Control flow escapes from PARALLEL" error,  thought it would result in multiple error messages for the same error. 
> > 
> > Hence was throwing the invalid branches to/from OpenMP structured blocks only for branching among the nested OpenMP constructs which were not handled as part of LabelEnforce such as the following case.
> > 
> >   !$omp parallel
> >   goto 10
> >   !$omp single
> >   10 print*, "OMP SINGLE"
> >   !$omp end single
> >   !$omp end parallel
> > 
> > 
> > Do we display** both Control flow escapes from 'CONSTRUCT' and invalid branches to/from OpenMP structured block** for cases where the control escapes out of OpenMP blocks such as the following?
> > 
> > 
> >   !$omp parallel
> >   goto 10
> >   !$omp end parallel
> >   10 print*, 10
> > 
> >Hence was throwing the invalid branches to/from OpenMP structured blocks only for branching among the nested OpenMP constructs which were not handled as part of LabelEnforce such as the following case.
> 
> I am assuming that nesting check should work but it is not happening here because CheckNoBranching is not called for the single construct.
> 
> > Do we display both Control flow escapes from 'CONSTRUCT' and invalid branches to/from OpenMP structured block for cases where the control escapes out of OpenMP blocks such as the following?
> 
> Ideally only one error should displayed.
Throwing the error 'invalid branches to/from OpenMP structured blocks' instead of Control flow escaping from the CONSTRUCT 


================
Comment at: flang/test/Semantics/omp-invalid-branch.f90:11
+  !$omp parallel
+  !ERROR: Control flow escapes from PARALLEL
+  open (10, file="myfile.dat", err=100)
----------------
praveen wrote:
> kiranchandramohan wrote:
> > praveen wrote:
> > > kiranchandramohan wrote:
> > > > Why doesn't the other error (invalid branches to/from OpenMP structured blocks) show here?
> > > @kiranchandramohan The error "invalid branches to/from OpenMP structured blocks" will be identified in the CheckLabelContext function for the cases where the control flow reaches out of OpenMP structured blocks. 
> > > 
> > > Because //we are already throwing// the "Control flow escapes from PARALLEL" error,  thought it would result in multiple error messages for the same error. 
> > > 
> > > Hence was throwing the invalid branches to/from OpenMP structured blocks only for branching among the nested OpenMP constructs which were not handled as part of LabelEnforce such as the following case.
> > > 
> > >   !$omp parallel
> > >   goto 10
> > >   !$omp single
> > >   10 print*, "OMP SINGLE"
> > >   !$omp end single
> > >   !$omp end parallel
> > > 
> > > 
> > > Do we display** both Control flow escapes from 'CONSTRUCT' and invalid branches to/from OpenMP structured block** for cases where the control escapes out of OpenMP blocks such as the following?
> > > 
> > > 
> > >   !$omp parallel
> > >   goto 10
> > >   !$omp end parallel
> > >   10 print*, 10
> > > 
> > >Hence was throwing the invalid branches to/from OpenMP structured blocks only for branching among the nested OpenMP constructs which were not handled as part of LabelEnforce such as the following case.
> > 
> > I am assuming that nesting check should work but it is not happening here because CheckNoBranching is not called for the single construct.
> > 
> > > Do we display both Control flow escapes from 'CONSTRUCT' and invalid branches to/from OpenMP structured block for cases where the control escapes out of OpenMP blocks such as the following?
> > 
> > Ideally only one error should displayed.
> Throwing the error 'invalid branches to/from OpenMP structured blocks' instead of Control flow escaping from the CONSTRUCT 
> I am assuming that nesting check should work but it is not happening here because CheckNoBranching is not called for the single construct.

We are calling the CheckNoBranching for the single construct. 
The error was not getting identified because in the parse-tree walk of the enclosing parallel construct , it does not identify the code between !$omp single and !$omp end single as that in a different scope and the label would be identified to be in the same scope as that of the statement causing the jump.




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

https://reviews.llvm.org/D92735