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

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan added a comment.

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



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


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

https://reviews.llvm.org/D92735