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

Sun Jan 31 08:06:26 PST 2021

kiranchandramohan added a comment.

In D92735#2532196 <https://reviews.llvm.org/D92735#2532196>, @praveen wrote:

>> I am not familiar with that. I can look up if required. But what I am suggesting is to do what is similar to the do loop checks like the following.
>>
>>   $ ./bin/flang ../flang/test/Semantics/doconcurrent03.f90
>>   ../flang/test/Semantics/doconcurrent03.f90:11:9: error: Control flow escapes from DO CONCURRENT
>>             goto 20
>>             ^^^^^^^
>>   ../flang/test/Semantics/doconcurrent03.f90:9:3: Enclosing DO CONCURRENT statement
>>       do 10 concurrent (i = 1:10)
>>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>   ../flang/test/Semantics/doconcurrent03.f90:18:3: branch into loop body from outside
>>       goto 30
>>       ^^^^^^^
>>   ../flang/test/Semantics/doconcurrent03.f90:9:3: the loop branched into
>>       do 10 concurrent (i = 1:10)
>>       ^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>   `
>
> @kiranchandramohan As per the current changes , the errors for the various cases are being displayed as follows
>
> **Branch into the OpenMP Structured block**
>
>   !$omp parallel
>   10 print*, 10
>   !$omp end parallel
>   goto 10
>    
>   ./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
>     ^^^^^^^^^^^^^
>
> **Branch out of OpenMP structured blocks**
>
>   !$omp parallel
>    goto 10
>   !$omp end parallel
>   10 print*, 10
>   
>   ./invalid-branching.f90:13:4: error: invalid branch to/from OpenMP structured block
>      goto 10
>      ^^^^^^^
>   ./invalid-branching.f90:15:1: Outside the enclosing PARALLEL directive
>   10 print*, 10
>   ^^^^^^^^^^^^^
>
> **Branch among 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
>     ^^^^^^^^^^^^^^
>
> //Since the second message in the above errors which indicates the label target source are not marked as **error** , I had not included them as plain comments in the test cases.//
>
>> BTW, for a branch leaving/entering a nested construct, how many errors are reported? Just to understand, Not requesting a change here.
>>
>>   !$omp parallel
>>   !$omp single
>>   !$omp end single
>>   goto 10
>>   !$omp end parallel
>>   
>>   10 print *, "Invalid"
>
> For the branch entering/leaving a nested construct , one error is reported by printing the innermost construct in which the error occured.
>
>   !$omp parallel
>   !$omp single
>   goto 10
>   !$omp end single
>   !$omp end parallel
>   10 print *, 10
>   
>   ./invalid-branching.f90:14:3: error: invalid branch to/from OpenMP structured block
>     goto 10
>     ^^^^^^^
>   ./invalid-branching.f90:17:3: Outside the enclosing SINGLE directive
>     10 print *, 10

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.

================
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,
----------------
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.

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

https://reviews.llvm.org/D92735