In D98603#2629758, @clementval wrote:

The clang-tidy seems to identify the case style as invalid for this function name GenerateFlangClauseParserKindMap
Any changes required regarding this ? All other functions in the file DirectiveEmitter.cpp follows the same naming convention.

Don't worry about this. All other TableGen backend follows this naming convention that's why we did it too.

@kiranchandramohan @clementval Thanks for the review . I have updated the test case for the Tablegen changes.

Update test case directive1.td for TableGen changes

@kiranchandramohan The test case flang/test/Semantics/resolve102.f90 is failing with the error messages possibly due to the commit

Add TODO and comments for the data structures

Check threadprivate variables in symbols with HostAssocDetails

Address review comments and rebase

Add check for cycle statements leaving OpenMP structured blocks

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.

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

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

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

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

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

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.

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.

Handle all the statements invloving labels and branches.

Address review comments

In D93091#2495365, @kiranchandramohan wrote:

Add a few tests to show that most restrictions do not apply inside a parallel construct nested inside a workshare construct.

Use parse-tree walk to check the validity of assignments and expressions in workshare construct .

@kiranchandramohan To make use of parse-tree walk to check all the assignments and expressions in OpenMP Workshare constructs , is it preferred to add a class in check-omp-structure.cpp specific for workshare or add a generic class in Semantics/tools.cpp to collect all the assignments and expressions and then iterate over all the assignments / expressions identified in the walk?

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?

In D92735#2488356, @kiranchandramohan wrote:

Need an answer here to proceed with review.

@kiranchandramohan Updated the patch to handle all the checks on workshare construct . Since the WhereConstruct and ForallConstructs have nested WhereStmt/WhereConstructs and ForAllStmt/ForallConstructs , separate functions are added to handle the same recursively.

1. Handle uses of non-elemental functions in the workshare construct
2. Handle the nested where/forall constructs enclosed in the workshare construct
3. Apply the restrictions on workshare to OpenMP critical construct enclosed in workshare construct

Update to latest syntax of Omp Reduction clause

@kiranchandramohan @sameeranjoshi Will the patch https://reviews.llvm.org/D93105 be committed to the master again?
If so , the function void GetSymbolsInDesignatorList(const std::list<parser::Designator> &, SymbolSourceMap &); would not be required.

Handle all the private symbols in enclosing context for firstprivate and lastprivate clauses

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.

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?

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

"Variables that appear in namelist statements, in variable format expressions, or in expressions for statement function definitions, may not appear in a firstprivate clause."

Rebase and apply clang-format

In D91920#2410307, @kiranchandramohan wrote:

In D91920#2410202, @praveen wrote:

@kiranchandramohan In the test file omp-clause-validity01.f90 , the following no symbol found errors are not being thrown.

!ERROR: Internal: no symbol found for 'i'
!$omp taskwait depend(sink:i-1)

!ERROR: Internal: no symbol found for 'first'
!$omp critical (first)

Since the errors related to copyprivate clause in omp-clause-validity01.f90 (line numbers 324 and 340 above) being thrown as part of the checks for copyprivate in resolve-directives.cpp are marked as fatal errors , the flag errorOnUnresolvedName_ is being set to false and the no symbol found error is not being thrown.

 RewriteMutator(SemanticsContext &context)
 : errorOnUnresolvedName_{!context.AnyFatalError()},
    messages_{context.messages()} {}

void RewriteMutator::Post(parser::Name &name) {
   if (!name.symbol && errorOnUnresolvedName_) {
     messages_.Say(name.source, "Internal: no symbol found for '%s'"_err_en_US,
       name.source);
    }
 }

Is it not necessary to throw the "no symbol found" error if there is any error marked as fatal while resolving the directives ?

Should all the checks related to copyprivate be moved to check-omp-structure.cpp?

Can you please suggest the approach to follow for these changes?

Thanks!

I think these checks can stay in the current file since they need to check the data sharing attributes.
For the depend clause I think the test need to be changed to a valid test. Is that OK @kiranktp.
For critical I believe we may not have done the semantic check and hence the symbol is not available. For this I think it is OK to comment out the check.

Is it not necessary to throw the "no symbol found" error if there is any error marked as fatal while resolving the directives ?

Is this OK @clementval ?

Addressed review comments and combined the test cases

Added a flag in symbol.h to identify the symbols in namelist

@kiranchandramohan Updated the reply for each of the inline comments.
Thanks!

Address review comments.

Rebase with latest master

@kiranchandramohan In the test file omp-clause-validity01.f90 , the following no symbol found errors are not being thrown.

Modified the logic to identify all the variables in namelist statements

Add test case omp-depend03.f90 for usage of coarray in the depend clause

Moved the pointer check to separate function as per review comment

Removed redundant code used to resolve threadprivate directive as per review comments.

Updated formatting

@kiranchandramohan @SouraVX Thanks for the review ! I have addressed the review comments as specified.

Address the review comments and update the summary.

Added positive test cases , formatting as per review comments

Updated formatting

@DavidTruby Thanks !

@kiranktp @DavidTruby Thanks for the review.

Add test cases for more OpenMP constructs