A few comments.

I believe the checks have to be made more precise. Please have a look at the comments.

LGTM. Thanks for the patience.

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

LGTM.

Requesting changes as per review comments/questions before. Thanks @clementval.

LGTM.

Thanks for the patience.

Looks mostly OK. Just one issue (FindEnclosingDirContext) to take a decision.

Need an answer here to proceed with review.

Thanks for this patch Alex. LGTM.

Two quick questions.

In D93213#2452004, @praveen wrote:

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

@kiranchandramohan I was not able to find the implementation for the "variable format expressions" . Do we have support for this part ?

A few comments.

I think the following three points can be dealt in other patches.
-> Attribute for inclusive upperbound/stop. If at some point in the future we decide to do some transformation/normalization then would this require initially creating a wsloop operation with inclusive attributes and bounds and then transforming this to a new wsloop operation without the attribute and normalized bounds and step? And is such a transformation OK with the MLIR flow?
-> Additional work in the OpenMPIRBuilder for handling negative step.
-> Changes to SCF for negative step (if required).

Why are you changing the tests while removing XFAIL? eg. omp-do05.f90 was supposed to check chunk_size.

@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

The following are not implemented. They also don't seem to be in the list of items to do. Do you know why they were not added?
-> The type and the rank of a list item that appears in a reduction clause must be valid for the combiner and initializer.
-> Additionally, the list item or the pointer component of the list item must not be deallocated, allocated, or pointer assigned within the region.
-> Additionally, the list item or the allocatable component of the list item must be neither deallocated nor allocated within the region.

Rebasing on main.

@yhegde Will the changes in this patch affect your reduction patch (https://reviews.llvm.org/D90697)?

LGTM.

First few comments.

LGTM.

For semantics should have a check where the following case is caught.
!$omp allocate[(list)] clause
!$omp allocate(list)
!$omp allocate(list)

allocate statement

Thanks for fixing this. LGTM.

A few comments.

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?

LGTM. Please wait for approval from @Meinersbur.

Thanks @ftynse for this patch. This is great and will speed us up by a month.

OK. please close.

Yes was referring to the 2), second one actually. So probably what check introduced in this patch is applicable as a semantic check. And I suppose the one which you were suggesting can be done at runtime. Also kindly suggest how I can go ahead with this patch.

I only saw the following one which puts some conditions for using the same assignment of logical iteration numbers to threads for two loops. If these conditions are not met it is just that the implementation can use a different assignment. Are you referring to something else?

LGTM. Thanks @praveen.

LGTM.

In D91982#2414233, @ftynse wrote:

In D91982#2414182, @kiranchandramohan wrote:

LGTM.

Do you need the lowering to LLVM IR also sometime soon?

Previously, did you mention something about a parallelisation strategy and SCF ops carrying an attribute to determine whether they should be lowered to OpenMP parallel loops.

@SouraVX we can discuss the lowering separately for Flang. Since we decided to represent the work-sharing loop as a loop like operation (omp.wsloop), lowering directly to the work-sharing loop from the parse tree will be the straight forward method. If there are difficulties with that we can think of lowering to fir.do_loop and then converting to omp.wsloop but I think this might need modification of fir.do_loop or addition of a directive like operation in FIR.

Thanks for the quick review!

I am in the process of writing a translation to LLVM IR, happy to use anything that is already available.

Do you need the lowering to LLVM IR also sometime soon?

Thanks for this patch.

@praveen can you reply inline in each comment whether you have made the changes suggested, reply to the question asked, or give your opinion? That way it will be easier for me to review.

Thanks. LGTM.

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!

Flang.Semantics::omp-clause-validity01.f90

Script:

: 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/test_errors.sh /mnt/disks/ssd0/agent/llvm-project/flang/test/Semantics/omp-clause-validity01.f90 /mnt/disks/ssd0/agent/llvm- project/build/tools/flang/test/Semantics/Output/omp-clause-validity01.f90.tmp /mnt/disks/ssd0/agent/llvm-project/build/bin/f18 -intrinsic-module-directory /mnt/disks/ssd0/agent/llvm-project/build/tools/flang/include/flang -fopenmp

No issues, please go ahead.

LGTM.

Looks OK to me. Couple of minor comments.

@kiranchandramohan does this seem ok from query which was raised in https://reviews.llvm.org/D89583#inline-836220 comment ?

LGTM.

I will provide my understanding below. Can also check with @huntergr @Meinersbur.

I guess the only remaining point to address is to handle return the pass/fail status to MLIR module translation.

Thanks @Meinersbur for your comments. I will have a look soon. I suspect these are only used now for generating string enum classes in mlir openmp.

Thanks @llitchev for this patch.

I) Are you handling only a subset of the restrictions here? I see the following in,

1. General restrictions.

-> allocate directives that appear in a target region must specify an allocator clause unless a requires directive with the dynamic_allocators clause is present in the same compilation unit.

I tested with make check-openmp and make check-clang on an AArch64 linux machine and both versions (D91002,D88252) pass.
If you were asking for a different kind of testing then please let me know.

@clementval had some changes for the OpenACC reduction operator in the following commit. I am not sure whether it is related to this issue.
https://reviews.llvm.org/D86296

LGTM.

ping.