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

[Praveen G via Phabricator via llvm-commits]

praveen added a comment.

In D92735#2518252 <https://reviews.llvm.org/D92735#2518252>, @kiranchandramohan wrote:

> In D92735#2514762 <https://reviews.llvm.org/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.



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



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

https://reviews.llvm.org/D92735