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

[praveen]

Summary

Add the checks for invalid branch to/from the OpenMP constructs with structured blocks

Resolve few related test cases marked as XFAIL.

[kiranchandramohan]

There is a labelEnforce class (https://github.com/llvm/llvm-project/blob/a2f922140f5380571fb74179f2bf622b3b925697/flang/lib/Semantics/tools.cpp#L1338) which is used for checks in do-concurrent and co-arrays. It is used to find control flow escaping from a construct. Can that be reused?
Function CheckBranchesIntoDoBody (https://github.com/llvm/llvm-project/blob/a2f922140f5380571fb74179f2bf622b3b925697/flang/lib/Semantics/resolve-labels.cpp#L845) in resolve-labels.cpp checks for branches into the body of a loop. Can this code be reused?

flang/lib/Semantics/check-omp-structure.h

76 ↗

(On Diff #309775)

Isn't this check true for all constructs with structured blocks like master, critical etc?

flang/test/Semantics/omp-parallel01.f90

1–2

Nit: The name of this test and the next has an extra 'l'.

[praveen]

Check invalid branch into/from all the openMP constructs with structured blocks

[praveen]

In D92735#2436164, @kiranchandramohan wrote:

There is a labelEnforce class (https://github.com/llvm/llvm-project/blob/a2f922140f5380571fb74179f2bf622b3b925697/flang/lib/Semantics/tools.cpp#L1338) which is used for checks in do-concurrent and co-arrays. It is used to find control flow escaping from a construct. Can that be reused?

@kiranchandramohan We can make use of labelEnforce class to identify the control flow escaping the construct . But it would not be useful to identify the control flow entering the OpenMP constructs and also it does not identify the transfer of control from one OpenMP construct to another such as this.

!$omp parallel
goto 10
a = a + b 
!$omp single
10 call sb()
!$omp end single
!$omp end parallel

Function CheckBranchesIntoDoBody (https://github.com/llvm/llvm-project/blob/a2f922140f5380571fb74179f2bf622b3b925697/flang/lib/Semantics/resolve-labels.cpp#L845) in resolve-labels.cpp checks for branches into the body of a loop. Can this code be reused?

Since the function CheckBranchesIntoDoBody does not follow the OpenMP constructs , I guess it would not be much useful in this case .

Please suggest.

flang/lib/Semantics/check-omp-structure.h

76 ↗

(On Diff #309775)

@kiranchandramohan Had only added the constructs that contained this restriction explicitly. Modified to handle this check for all constructs with structured blocks .

Thanks!

flang/test/Semantics/omp-parallel01.f90

1–2

Removed the extra 'l' . Thanks

[sameeranjoshi]

Please suggest.

Please see D93447 if it helps in this patch.
I am unaware of and haven't looked into how we can reuse code for branching inside maybe you can think/would be better how we could reuse the code already present.
Also do look into NoBranchingEnforce if we can reuse.

[praveen]

In D92735#2459786, @sameeranjoshi wrote:

Please suggest.

Please see D93447 if it helps in this patch.
I am unaware of and haven't looked into how we can reuse code for branching inside maybe you can think/would be better how we could reuse the code already present.
Also do look into NoBranchingEnforce if we can reuse.

@sameeranjoshi Yes . I had initially thought of the same change as in https://reviews.llvm.org/D93447 using the existing 'NoBranchingEnforce' as it would handle the branching out of the construct part . But since it wasnt able to identify the branching into the constructs , I made the changes in resolve-directives.cpp as part of this patch.

[kiranchandramohan]

@kiranchandramohan We can make use of labelEnforce class to identify the control flow escaping the construct . But it would not be useful to identify the control flow entering the OpenMP constructs and also it does not identify the transfer of control from one OpenMP construct to another such as this.
!$omp parallel
goto 10
a = a + b
!$omp single
10 call sb()
!$omp end single
!$omp end parallel

Not clear what the issue is here. Is it that it will be inefficient to call enforce for each nested construct?

Function CheckBranchesIntoDoBody (https://github.com/llvm/llvm-project/blob/a2f922140f5380571fb74179f2bf622b3b925697/flang/lib/Semantics/resolve-labels.cpp#L845) in resolve-labels.cpp checks for branches into the body of a loop. Can this code be reused?

Since the function CheckBranchesIntoDoBody does not follow the OpenMP constructs , I guess it would not be much useful in this case . Please suggest.

The CheckBranchesIntoDoBody takes in a list of branches, labels, location of bodies of constructs. This seemed to be a generic function which can be moved to Semantics/tools.cpp and reused.

void CheckBranchesIntoDoBody(const SourceStmtList &branches,
    const TargetStmtMap &labels, const IndexList &loopBodies,
    SemanticsContext &context)

The reason I asked to look at reusing existing code is because labelEnforce class seems to be checking for labels arising for more statements (see below). It was not clear to me whether you were missing some or whether some are not allowed in OpenMP.

void LabelEnforce::Post(const parser::GotoStmt &gotoStmt) {
void LabelEnforce::Post(const parser::ComputedGotoStmt &computedGotoStmt) {
void LabelEnforce::Post(const parser::ArithmeticIfStmt &arithmeticIfStmt) {
void LabelEnforce::Post(const parser::AssignStmt &assignStmt) {
void LabelEnforce::Post(const parser::AssignedGotoStmt &assignedGotoStmt) {
void LabelEnforce::Post(const parser::AltReturnSpec &altReturnSpec) {
void LabelEnforce::Post(const parser::ErrLabel &errLabel) {
void LabelEnforce::Post(const parser::EndLabel &endLabel) {
void LabelEnforce::Post(const parser::EorLabel &eorLabel) {

[kiranchandramohan]

Need an answer here to proceed with review.

[praveen]

In D92735#2488356, @kiranchandramohan wrote:

Need an answer here to proceed with review.

@kiranchandramohan I am trying to resuse the function CheckBranchesIntoDoBody . Will update the patch regarding the same.

@sameeranjoshi Will you be merging the related changes https://reviews.llvm.org/D93447 that makes use of LabelEnforce ?

[sameeranjoshi]

In D92735#2490685, @praveen wrote:

In D92735#2488356, @kiranchandramohan wrote:

Need an answer here to proceed with review.

@kiranchandramohan I am trying to resuse the function CheckBranchesIntoDoBody . Will update the patch regarding the same.

@sameeranjoshi Will you be merging the related changes https://reviews.llvm.org/D93447 that makes use of LabelEnforce ?

I was waiting for action based on the approach we follow from the current patch.
If all statements from LabelEnforce are allowed inside an openmp block, I can merge the pending patch?

[praveen]

In D92735#2492674, @sameeranjoshi wrote:

In D92735#2490685, @praveen wrote:

In D92735#2488356, @kiranchandramohan wrote:

Need an answer here to proceed with review.

@kiranchandramohan I am trying to resuse the function CheckBranchesIntoDoBody . Will update the patch regarding the same.

@sameeranjoshi Will you be merging the related changes https://reviews.llvm.org/D93447 that makes use of LabelEnforce ?

I was waiting for action based on the approach we follow from the current patch.
If all statements from LabelEnforce are allowed inside an openmp block, I can merge the pending patch?

@sameeranjoshi I do not see any restrictions on not allowing the statements part of 'LabelEnforce' inside the OpenMP block as per the OpenMP 4.5 documentation.

[praveen]

Handle all the statements invloving labels and branches.

[praveen]

@kiranchandramohan @sameeranjoshi

I tried to make use of the CheckBranchesIntoDoBody function, but since it involves the use of multiple functions and types defined in resolve-labels.cpp such as using ProxyForScope, LabeledStatementInfoTuplePOD, SourceStatementInfoTuplePOD and also the labels, scopes and other details are being identified as part of the walk in ParseTreeAnalyzer class, it would be more complex to reuse these functions.

Hence I felt adding the logic in resolve-directives.cpp to identify the control flow to / from OpenMP structured blocks would be simple . Also handled the checks for all the statements involving labels as in 'LabelEnforce' class except AssignStmt.

void LabelEnforce::Post(const parser::AssignStmt &);
The check involving AssignStmt in LabelEnforce may not be suitable as part of the control flow checks for OpenMP constructs.

integer :: b
!$omp parallel
assign 5 to b
!$omp end parallel

5 print *, 5

In the above example, we detect this as an error as part of the 'LabelEnforce' check.

Can we make use of the function CheckLabelContext added as part of this change itself to check the control flow escaping the OpenMP constructs ?

[kiranchandramohan]

In D92735#2514762, @praveen wrote:

@kiranchandramohan @sameeranjoshi

I tried to make use of the CheckBranchesIntoDoBody function, but since it involves the use of multiple functions and types defined in resolve-labels.cpp such as using ProxyForScope, LabeledStatementInfoTuplePOD, SourceStatementInfoTuplePOD and also the labels, scopes and other details are being identified as part of the walk in ParseTreeAnalyzer class, it would be more complex to reuse these functions.

Hence I felt adding the logic in resolve-directives.cpp to identify the control flow to / from OpenMP structured blocks would be simple .

OK if it is difficult or too specialized for Do loops then no need to reuse CheckBranchesIntoDoBody.

Also handled the checks for all the statements involving labels as in 'LabelEnforce' class except AssignStmt.

void LabelEnforce::Post(const parser::AssignStmt &);
The check involving AssignStmt in LabelEnforce may not be suitable as part of the control flow checks for OpenMP constructs.

integer :: b
!$omp parallel
assign 5 to b
!$omp end parallel

5 print *, 5

In the above example, we detect this as an error as part of the 'LabelEnforce' check.

Can we make use of the function CheckLabelContext added as part of this change itself to check the control flow escaping the OpenMP constructs ?

Can we remove void LabelEnforce::Post(const parser::AssignStmt &); from LabelEnforce? Are you saying it is not applicable in this context but applicable in other contexts?

You might want to do a thorough check to ensure that the branch into checks are called for all OpenMP constructs. I think it is now not called at least for Critical, Ordered constructs.

flang/test/Semantics/omp-invalid-branch.f90

11 ↗

(On Diff #318435)

Why doesn't the other error (invalid branches to/from OpenMP structured blocks) show here?

[kiranchandramohan]

flang/lib/Semantics/resolve-directives.cpp

236

Add a few comments on what is done on seeing a statement with a label.

360

Add a few comments on what is done on seeing a statement which can cause a jump to a label.

1355

Can the error message also contain the entry point into the OpenMP region? CheckBranchesIntoDoBody prints error messages like that

[praveen]

In D92735#2518252, @kiranchandramohan wrote:

In D92735#2514762, @praveen wrote:

@kiranchandramohan @sameeranjoshi

I tried to make use of the CheckBranchesIntoDoBody function, but since it involves the use of multiple functions and types defined in resolve-labels.cpp such as using ProxyForScope, LabeledStatementInfoTuplePOD, SourceStatementInfoTuplePOD and also the labels, scopes and other details are being identified as part of the walk in ParseTreeAnalyzer class, it would be more complex to reuse these functions.

Hence I felt adding the logic in resolve-directives.cpp to identify the control flow to / from OpenMP structured blocks would be simple .

OK if it is difficult or too specialized for Do loops then no need to reuse CheckBranchesIntoDoBody.

Also handled the checks for all the statements involving labels as in 'LabelEnforce' class except AssignStmt.

void LabelEnforce::Post(const parser::AssignStmt &);
The check involving AssignStmt in LabelEnforce may not be suitable as part of the control flow checks for OpenMP constructs.

integer :: b
!$omp parallel
assign 5 to b
!$omp end parallel

5 print *, 5

In the above example, we detect this as an error as part of the 'LabelEnforce' check.

Can we make use of the function CheckLabelContext added as part of this change itself to check the control flow escaping the OpenMP constructs ?

Can we remove void LabelEnforce::Post(const parser::AssignStmt &); from LabelEnforce? Are you saying it is not applicable in this context but applicable in other contexts?

@kiranchandramohan We can maybe remove void LabelEnforce::Post(const parser::AssignStmt &); check from LabelEnforce as it detects the error when there is only AssignStmt.
The control flow escaping from 'CONSTRUCT' would make sense for parser::AssignStmt only when the assigned label is used in the goto stmt (AssignedGoto) in the same context as below.

integer n
!$omp parallel 
assign 1 to n
goto n
!$omp end parallel
1 print *,  1

But it does not detect the error in the following case since the AssignedGoto check does not identify the context of the variable 'n'

integer n
assign 1 to n
!$omp parallel 
goto n
!$omp end parallel
1 print *,  1

The assigned goto check void LabelEnforce::Post(const parser::AssignedGotoStmt &) detects the errors only when specified with the list of labels as below.

integer n
assign 1 to n
!$omp parallel 
goto n (1,2)
!$omp end parallel
1 print *,  1
2 print *,  2

This behaviour is same with the do concurrent and critical constructs which makes use of LabelEnforce class.

You might want to do a thorough check to ensure that the branch into checks are called for all OpenMP constructs. I think it is now not called at least for Critical, Ordered constructs.

I will check all the OpenMP constructs thoroughly to see if the checks works as expected for all constructs .
The check currently works for all the directives which have DirContext object.

The branch into check for Critical construct will work if we handle Pre(const parser::OpenMPCriticalConstruct &) and invoke PushContext(). Shall I handle it as part of this patch ?
I see a patch https://reviews.llvm.org/D93051 from @sameeranjoshi for critical construct with which this check would work.

flang/test/Semantics/omp-invalid-branch.f90

11 ↗

(On Diff #318435)

@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

[kiranchandramohan]

The branch into check for Critical construct will work if we handle Pre(const parser::OpenMPCriticalConstruct &) and invoke PushContext(). Shall I handle it as part of this patch ?
I see a patch https://reviews.llvm.org/D93051 from @sameeranjoshi for critical construct with which this check would work.

I think you can handle in this patch.

If your patch is able to handle all cases then may be it is better to remove the code added by @sameeranjoshi in checkNoBranching. While common code is better, in this case i believe there will be some compilation time impact due to the fact that,

1. resolve-directives is called twice.
2. check-no-branching calls collecting labels code and then checks the branch to the labels. This is traversing all the nodes twice.
3. for nested constructs it has to be called for each construct. Hence the same innermost labelled construct might be traversed as many times as the nesting.

@sameeranjoshi let us know if you disagree or see any issues. Thanks (as always) for your work and pro-activity.

flang/test/Semantics/omp-invalid-branch.f90

11 ↗

(On Diff #318435)

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.

[praveen]

Use the logic in resolve-directives.cpp instead of 'LabelEnforce' to identify control flow escaping the OpenMP constructs.

Handle omp taskgroup and omp critical constructs to check the invalid entry or branch to/from these constructs.

Resolve omp-taskloop01.f90 which was failing due to taskgroup not being handled.

[praveen]

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

flang/lib/Semantics/resolve-directives.cpp

236

Done

360

Done

1355

Yes . Modified the error messages to contain the entry point into the OpenMP region

flang/test/Semantics/omp-invalid-branch.f90

11 ↗

(On Diff #318435)

Throwing the error 'invalid branches to/from OpenMP structured blocks' instead of Control flow escaping from the CONSTRUCT

11 ↗

(On Diff #318435)

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.

[kiranchandramohan]

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

flang/lib/Semantics/resolve-directives.cpp

1365

If this is only about branches leaving the OpenMP structured block then can it be renamed to
"invalid branch leaving and OpenMP structured block"?

flang/test/Semantics/omp-invalid-branch.f90

11 ↗

(On Diff #318435)

!$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.

[praveen]

In 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 as separate messages ?

flang/lib/Semantics/resolve-directives.cpp

1365

@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"

flang/test/Semantics/omp-invalid-branch.f90

11 ↗

(On Diff #318435)

@kiranchandramohan Thanks for the detailed explanation.

[kiranchandramohan]

In D92735#2530901, @praveen wrote:

In 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 as separate messages ?

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)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^
`

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"

flang/lib/Semantics/resolve-directives.cpp

1365

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

[praveen]

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

flang/lib/Semantics/resolve-directives.cpp

1365

@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
  ^^^^^^^^^^^^^^

[kiranchandramohan]

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

flang/lib/Semantics/resolve-directives.cpp

1365

Thanks @praveen for the detailed listing. I understand it now.

I would still prefer separate errors as you listed above.

[praveen]

Modify error messages to identify branch into or branch out of openmp structued blocks

[praveen]

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

flang/lib/Semantics/resolve-directives.cpp

1365

@kiranchandramohan Thanks. I have updated the error messages

[praveen]

Add check for cycle statements leaving OpenMP structured blocks

[kiranchandramohan]

LGTM. Thanks for the patch and the patience.

flang/lib/Semantics/check-omp-structure.cpp

HasInvalidWorksharingNesting(beginDir.source,
             llvm::omp::Directive::OMPD_task,
             llvm::omp::Directive::OMPD_taskloop,
             llvm::omp::Directive::OMPD_critical,
             llvm::omp::Directive::OMPD_ordered,
             llvm::omp::Directive::OMPD_atomic,
             llvm::omp::Directive::OMPD_master});
     PushContextAndClauseSets(beginDir.source, llvm::omp::Directive::OMPD_do);
   }
+
+  if (const auto &doConstruct{
+          std::get<std::optional<parser::DoConstruct>>(x.t)}) {
+    const auto &doBlock{std::get<parser::Block>(doConstruct->t)};
+    CheckNoBranching(doBlock, beginDir.v, beginDir.source);
+  }
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPLoopConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(const parser::OmpEndLoopDirective &x) {
   const auto &dir{std::get<parser::OmpLoopDirective>(x.t)};
void OmpStructureChecker::Enter(const parser::OpenMPBlockConstruct &x) {
   const auto &endBlockDir{std::get<parser::OmpEndBlockDirective>(x.t)};
   const auto &beginDir{std::get<parser::OmpBlockDirective>(beginBlockDir.t)};
   const auto &endDir{std::get<parser::OmpBlockDirective>(endBlockDir.t)};
   const parser::Block &block{std::get<parser::Block>(x.t)};
 
   CheckMatching<parser::OmpBlockDirective>(beginDir, endDir);
 
   PushContextAndClauseSets(beginDir.source, beginDir.v);
-
-  switch (beginDir.v) {
-  case llvm::omp::OMPD_parallel:
-    CheckNoBranching(block, llvm::omp::OMPD_parallel, beginDir.source);
-    break;
-  default:
-    break;
-  }
+  CheckNoBranching(block, beginDir.v, beginDir.source);
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPBlockConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPSectionsConstruct &x) {
   const auto &beginSectionsDir{
       std::get<parser::OmpBeginSectionsDirective>(x.t)};
   const auto &endSectionsDir{std::get<parser::OmpEndSectionsDirective>(x.t)};
   const auto &beginDir{
       std::get<parser::OmpSectionsDirective>(beginSectionsDir.t)};
   const auto &endDir{std::get<parser::OmpSectionsDirective>(endSectionsDir.t)};
   CheckMatching<parser::OmpSectionsDirective>(beginDir, endDir);
 
   PushContextAndClauseSets(beginDir.source, beginDir.v);
+
+  const auto &sectionBlocks{std::get<parser::OmpSectionBlocks>(x.t)};
+  for (const auto &block : sectionBlocks.v) {
+    CheckNoBranching(block, beginDir.v, beginDir.source);
+  }
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPSectionsConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(const parser::OmpEndSectionsDirective &x) {
   const auto &dir{std::get<parser::OmpSectionsDirective>(x.t)};

 
 void OmpStructureChecker::Leave(const parser::OpenMPCancelConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(const parser::OpenMPCriticalConstruct &x) {
   const auto &dir{std::get<parser::OmpCriticalDirective>(x.t)};
   PushContextAndClauseSets(dir.source, llvm::omp::Directive::OMPD_critical);
+
+  const auto &block{std::get<parser::Block>(x.t)};
+  CheckNoBranching(block, llvm::omp::Directive::OMPD_critical, dir.source);
 }
 
 void OmpStructureChecker::Leave(const parser::OpenMPCriticalConstruct &) {
   dirContext_.pop_back();
 }
 
 void OmpStructureChecker::Enter(
     const parser::OpenMPCancellationPointConstruct &x) {

flang/lib/Semantics/resolve-directives.cpp

 public:
   explicit OmpAttributeVisitor(SemanticsContext &context)
       : DirectiveAttributeVisitor(context) {}
 
   template <typename A> void Walk(const A &x) { parser::Walk(x, *this); }
   template <typename A> bool Pre(const A &) { return true; }
   template <typename A> void Post(const A &) {}
 
+  template <typename A> bool Pre(const parser::Statement<A> &statement) {
+    currentStatementSource_ = statement.source;
+    if (statement.label) {
+      auto label{statement.label.value()};
+      DirContext *thisContext{nullptr};
+      if (!dirContext_.empty()) {
+        thisContext = &GetContext();
+      }
+      targetLabels_.emplace(label, thisContext);
+      auto range{gotoLabels_.equal_range(label)};
+      for (auto it{range.first}; it != range.second; ++it) {
+        CheckGotoContext(it->second.first, it->second.second, thisContext);
+      }
+    }
+    return true;
+  }
+
+  bool Pre(const parser::GotoStmt &stmt) {
+    auto label{stmt.v};
+    DirContext *thisContext{nullptr};
+    if (!dirContext_.empty()) {
+      thisContext = &GetContext();
+    }
+    gotoLabels_.emplace(
+        label, std::make_pair(currentStatementSource_, thisContext));
+    auto it{targetLabels_.find(label)};
+    if (it != targetLabels_.end()) {
+      CheckGotoContext(currentStatementSource_, thisContext, it->second);
+    }
+    return true;
+  }
+
+  bool Pre(const parser::ComputedGotoStmt &stmt) {
+    DirContext *thisContext{nullptr};
+    if (!dirContext_.empty()) {
+      thisContext = &GetContext();
+    }
+    for (const auto &label : std::get<std::list<parser::Label>>(stmt.t)) {
+      gotoLabels_.emplace(
+          label, std::make_pair(currentStatementSource_, thisContext));
+      auto it{targetLabels_.find(label)};
+      if (it != targetLabels_.end()) {
+        CheckGotoContext(currentStatementSource_, thisContext, it->second);
+      }
+    }
+    return true;
+  }
+
   bool Pre(const parser::SpecificationPart &x) {
     Walk(std::get<std::list<parser::OpenMPDeclarativeConstruct>>(x.t));
     return true;
   }
 
   bool Pre(const parser::OpenMPBlockConstruct &);
   void Post(const parser::OpenMPBlockConstruct &);
 
public:
   }
   bool Pre(const parser::OmpAlignedClause &x) {
     const auto &alignedNameList{std::get<std::list<parser::Name>>(x.t)};
     ResolveOmpNameList(alignedNameList, Symbol::Flag::OmpAligned);
     return false;
   }
   void Post(const parser::Name &);
 
   const parser::OmpClause *associatedClause{nullptr};
   void SetAssociatedClause(const parser::OmpClause &c) {
     associatedClause = &c;
   }
   const parser::OmpClause *GetAssociatedClause() { return associatedClause; }
 
 private:
   std::int64_t GetAssociatedLoopLevelFromClauses(const parser::OmpClauseList &);
 
private:
   static constexpr Symbol::Flags ompFlagsRequireMark{
       Symbol::Flag::OmpThreadprivate};
 
   static constexpr Symbol::Flags dataCopyingAttributeFlags{
       Symbol::Flag::OmpCopyIn};
 
   std::vector<const parser::Name *> allocateNames_; // on one directive
   SymbolSet privateDataSharingAttributeObjects_; // on one directive
+  std::map<const parser::Label, DirContext *> targetLabels_;
+  std::multimap<const parser::Label, std::pair<parser::CharBlock, DirContext *>>
+      gotoLabels_;
+  parser::CharBlock currentStatementSource_;
 
   void AddAllocateName(const parser::Name *&object) {
     allocateNames_.push_back(object);
   }
   void ClearAllocateNames() { allocateNames_.clear(); }
 
   void AddPrivateDataSharingAttributeObjects(SymbolRef object) {
     privateDataSharingAttributeObjects_.insert(object);
void CheckMultipleAppearances(
       const parser::Name &, const Symbol &, Symbol::Flag);
 
   void CheckDataCopyingClause(
       const parser::Name &, const Symbol &, Symbol::Flag);
 
   void CheckAssocLoopLevel(std::int64_t level, const parser::OmpClause *clause);
   void CheckObjectInNamelist(
       const parser::Name &, const Symbol &, Symbol::Flag);
+  void CheckGotoContext(const parser::CharBlock, DirContext *, DirContext *);
 };
 
 template <typename T>
 bool DirectiveAttributeVisitor<T>::HasDataSharingAttributeObject(
     const Symbol &object) {
   auto it{dataSharingAttributeObjects_.find(object)};
   return it != dataSharingAttributeObjects_.end();
 }
if (symbol.GetUltimate().test(Symbol::Flag::InNamelist)) {
     else if (ompFlag == Symbol::Flag::OmpLastPrivate)
       clauseName = "LASTPRIVATE";
     context_.Say(name.source,
         "Variable '%s' in NAMELIST cannot be in a %s clause"_err_en_US,
         name.ToString(), clauseName.str());
   }
 }
 
+void OmpAttributeVisitor::CheckGotoContext(const parser::CharBlock source,
+    DirContext *gotoContext, DirContext *labelContext) {
+  if (labelContext && !gotoContext) {
+    context_.Say(source, "invalid entry to OpenMP structured block"_err_en_US);
+  } else if (gotoContext &&
+      (!labelContext || gotoContext->scope != labelContext->scope)) {
+    context_.Say(
+        source, "invalid branch to/from OpenMP structured block"_err_en_US);
+  }
+}
+
 } // namespace Fortran::semantics

flang/test/Semantics/omp-clause-validity01.f90

!                        allocate-clause
   outofparallel: do k =1, 10
   !$omp parallel
   !$omp do
   outer: do i=0, 10
     inner: do j=1, 10
       exit
       exit outer
       !ERROR: EXIT to construct 'outofparallel' outside of PARALLEL construct is not allowed
+      !ERROR: EXIT to construct 'outofparallel' outside of DO construct is not allowed
       exit outofparallel
     end do inner
   end do outer
   !$end omp do
   !$omp end parallel
   end do outofparallel
 
 ! 2.7.1  do-clause -> private-clause |

flang/test/Semantics/omp-do07.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
-! XFAIL:*
 
 ! OpenMP Version 4.5
 ! 2.7.1 Loop Construct
 ! No statement in the associated loops other than the DO statements
 ! can cause a branch out of the loops
 
 program omp_do
   integer i, j, k

flang/test/Semantics/omp-parallel01.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
-! XFAIL: *
 
 ! OpenMP Version 4.5
 ! 2.5 parallel construct.
 ! A program that branches into or out of a parallel region
 ! is non-conforming.
 
 program omp_parallel
   integer i, j, k
 

flang/test/Semantics/omp-parallel02.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
-! XFAIL: *
 
 ! OpenMP Version 4.5
 ! 2.5 parallel construct.
 ! A program that branches into or out of a parallel region
 ! is non-conforming.
 
 program omp_parallel
   integer i, j, k
 
   !ERROR: invalid entry to OpenMP structured block
   goto 10
 
   !$omp parallel
   do i = 1, 10
     do j = 1, 10
       print *, "Hello"
+      !ERROR: STOP statement is not allowed in a PARALLEL construct
       10 stop
     end do
   end do
   !$omp end parallel
 
 end program omp_parallel

flang/test/Semantics/omp-simd01.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
-! XFAIL: *
 
 ! OpenMP Version 4.5
 ! 2.8.1 simd Construct
 ! A program that branches into or out of a simd region is non-conforming.
 
 program omp_simd
   integer i, j
 

flang/test/Semantics/omp-task01.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
-! XFAIL: *
 
 ! OpenMP Version 4.5
 ! 2.9.1 task Construct
 ! Invalid entry to OpenMP structured block.
 
 recursive subroutine traverse ( P )
   type Node
     type(Node), pointer :: left, right
   end type Node
 
   type(Node) :: P
 
   !ERROR: invalid entry to OpenMP structured block
   goto 10
 
   if (associated(P%left)) then
     !$omp task
     call traverse(P%left)
+    !ERROR: STOP statement is not allowed in a TASK construct
     10 stop
     !$omp end task
   endif
 
   if (associated(P%right)) then
     !$omp task
     call traverse(P%right)
     !$omp end task
     endif
   call process ( P )
 
  end subroutine traverse

flang/test/Semantics/omp-taskloop02.f90

 ! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
-! XFAIL: *
 
 ! OpenMP Version 4.5
 ! 2.9.2 taskloop Construct
 ! Invalid entry to OpenMP structured block.
 
 program omp_taskloop
   integer i , j