[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.

In D92735#2530795 <https://reviews.llvm.org/D92735#2530795>, @kiranchandramohan wrote:

> Can the error messages be updated to check for the entire message, both the place where it is entering and leaving (or viceversa)?

Currently , the error message with the source of the statement causing the branch is thrown as a fatal error and the error message with source of the target label printed as part of the Attach() is not being marked as fatal .

Should both the error messages be marked as fatal and the error messages be updated accordingly ?



================
Comment at: flang/lib/Semantics/resolve-directives.cpp:1556
+    context_
+        .Say(source, "invalid branch to/from OpenMP structured block"_err_en_US)
+        .Attach(target, "Outside the enclosing %s directive"_en_US,
----------------
kiranchandramohan wrote:
> If this is only about branches leaving the OpenMP structured block then can it be renamed to
> "invalid branch leaving and OpenMP structured block"?
@kiranchandramohan The error "invalid branch to/from OpenMP structured block" is thrown for both the cases where there is a branch out of the OpenMP structured block and also branch among the OpenMP structured blocks in which case it leaves one construct and enters another one.

Can we split the above cases to contain different error messages as below


**Branch out of the OpenMP blocks** -   "invalid branch leaving an OpenMP structured block"


**Branch among the OpenMP structured blocks** - 
 "invalid branch to an OpenMP structured block in SOURCE directive " 
 "invalid branch from an OpenMP structured block in TARGET directive"



================
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:
> > 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.
> > 
> > 
> ```
> !$omp parallel
> goto 10
> !$omp single
> 10 print*, "OMP SINGLE"
> !$omp end single
> !$omp end parallel
> ```
> 
> For the above case label enforce is not printing out an error because there is no control flow leaving an OpenMP region here. It always stays inside parallel. The error here is that there is a branch entering the single construct  which is not what label enforce is concerned with.
@kiranchandramohan Thanks for the detailed explanation.


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

https://reviews.llvm.org/D92735