Page MenuHomePhabricator

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

Authored by praveen on Dec 6 2020, 11:08 AM.

Details

Summary

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

Resolve few related test cases marked as XFAIL.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
praveen requested review of this revision.Dec 6 2020, 11:08 AM
Herald added a project: Restricted Project. · View Herald Transcript
kiranchandramohan requested changes to this revision.Dec 6 2020, 2:35 PM

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
0–1

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

This revision now requires changes to proceed.Dec 6 2020, 2:35 PM
praveen updated this revision to Diff 312141.Dec 16 2020, 12:48 AM
praveen edited the summary of this revision. (Show Details)

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

praveen marked 2 inline comments as done.Dec 16 2020, 1:05 AM

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
0–1

Removed the extra 'l' . Thanks

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 marked 2 inline comments as done.Dec 17 2020, 2:15 AM

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 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 requested changes to this revision.Jan 8 2021, 7:26 PM

Need an answer here to proceed with review.

This revision now requires changes to proceed.Jan 8 2021, 7:26 PM

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 ?

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?

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 updated this revision to Diff 318435.Jan 22 2021, 12:07 AM

Handle all the statements invloving labels and branches.

praveen added a comment.EditedJan 22 2021, 12:16 AM

@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 requested changes to this revision.Jan 24 2021, 8:13 AM

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

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

This revision now requires changes to proceed.Jan 24 2021, 8:13 AM
flang/lib/Semantics/resolve-directives.cpp
245

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

371

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

1546

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

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

@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

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
12

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 updated this revision to Diff 320049.Jan 28 2021, 10:23 PM

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 marked 5 inline comments as done.Jan 28 2021, 10:39 PM

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

Done

371

Done

1546

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

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

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

12

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 requested changes to this revision.Jan 29 2021, 8:25 AM

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
1556

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

This revision now requires changes to proceed.Jan 29 2021, 8:25 AM
praveen marked 5 inline comments as done.EditedJan 29 2021, 9:53 AM

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
1556

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

@kiranchandramohan Thanks for the detailed explanation.

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
1556

@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 marked an inline comment as done.Jan 30 2021, 7:32 AM

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
1556

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

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
1556

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

I would still prefer separate errors as you listed above.

praveen updated this revision to Diff 320453.Feb 1 2021, 6:40 AM

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

praveen marked 3 inline comments as done.Feb 1 2021, 6:42 AM

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
1556

@kiranchandramohan Thanks. I have updated the error messages

praveen updated this revision to Diff 320810.Feb 2 2021, 9:32 AM
praveen marked an inline comment as done.

Add check for cycle statements leaving OpenMP structured blocks

kiranchandramohan accepted this revision.Feb 2 2021, 2:06 PM

LGTM. Thanks for the patch and the patience.

This revision is now accepted and ready to land.Feb 2 2021, 2:06 PM