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

[Praveen G via Phabricator via llvm-commits]

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

> Thanks for the detailed listing.  I think the second message in the do loop checks (non-openmp) is also not listed as an error. But they still check for that in the tests.
> In short,
>
> 1. you can follow what they do for the do loop checks (non-openmp)
> 2. Add the two separate messages in the tests.

@kiranchandramohan I have updated the error messages and the test cases to check for two separate messages in the tests



================
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:
> praveen wrote:
> > kiranchandramohan wrote:
> > > praveen wrote:
> > > > 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"
> > > > 
> > > @praveen In my opinion we should consider only two cases,
> > > 1) Branch out of an openmp structured block.
> > > 2) Branch into an openmp structured block.
> > > 
> > > either (1) or (2) or both can happen. But we do not need to distinguish further like whether the branch is among or not among OpenMP structured blocks. If I am missing a point here, please let me know.
> > @kiranchandramohan Yes . As you have pointed out , we should consider only two cases - branch into and branch out of the OpenMP structured blocks. 
> > 
> >   !$omp parallel
> >   10 print *, 10
> >   !$omp end parallel
> > 
> >   !$omp parallel
> >   goto 10
> >   !$omp end parallel
> > 
> >   ./invalid-branching.f90:17:3: error: invalid branch to/from OpenMP structured block
> >     goto 10
> >     ^^^^^^^
> >   ./invalid-branching.f90:13:3: Outside the enclosing PARALLEL directive
> >     10 print *, 10
> >     ^^^^^^^^^^^^^^
> > 
> > I had followed the error reporting from gfortran which displays a single error 
> > "invalid branch to/from OpenMP structured block" . 
> > 
> > Currently , for the above case , we are displaying a single error similar to gfortran. 
> > 
> > Please suggest if it would be appropriate to display two errors both -  **branch into and branch out errors separately** for the two constructs in cases such as the above ones as shown below?
> > 
> >   !$omp parallel
> >   10 print *, 10
> >   !$omp end parallel
> > 
> >   !$omp parallel
> >   goto 10
> >   !$omp end parallel
> > 
> >   ./invalid-branching.f90:15:1: error: invalid entry to OpenMP structured block
> >   goto 10
> >   ^^^^^^^
> >   ./invalid-branching.f90:13:3: In the enclosing PARALLEL directive
> >     10 print*, 10
> >     ^^^^^^^^^^^^^
> > 
> >   ./invalid-branching.f90:15:1: error: invalid branch leaving an OpenMP structured block
> >     goto 10
> >     ^^^^^^^
> >   ./invalid-branching.f90:13:3: Outside the enclosing PARALLEL directive
> >     10 print *, 10
> >     ^^^^^^^^^^^^^^
> > 
> > 
> > 
> > 
> > 
> > 
> Thanks @praveen for the detailed listing. I understand it now.
> 
> I would still prefer separate errors as you listed above.
@kiranchandramohan Thanks. I have updated the error messages


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

https://reviews.llvm.org/D92735