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.

Would need a response for questions above to proceed to acceptance.

Would need a response for questions above to proceed to acceptance.

@yhegde please submit. If you require any inputs then please let me know. This will be helpful for D91159.

@yhegde can this be submitted? If you require inputs from me please let me know.

@yhegde Can this be submitted? If you need any further inputs please let me know.

OK. LGTM.

Nit: Please reply inline and mark suggested changes as done.

Addressed formatting comments and description changes suggested by @ftynse.

LGTM.

LGTM.

Thanks for this patch.

Thanks for the cleanup. If you can remove the macros in a later patch that would be great.

LGTM.

LGTM. Have a nit comment inline about how to construct the message.

LGTM.

Two more questions.

Thanks @sameeranjoshi for this patch. Couple of questions inline.

LGTM.
Please wait for approval from @clementval after uppercasing the clause names.

A few questions. Otherwise LGTM.

CMake changes were made in https://reviews.llvm.org/D88351.

What benefit do you see compared to the current approach?

We cannot put a breakpoint in macros.

Another way would be to classify various OpenMP clauses similar to what is done in PFT.
https://github.com/llvm/llvm-project/blob/09662eeb46f76cda0a8d1102d8f67f7ad8ad64de/flang/include/flang/Lower/PFTBuilder.h#L115
https://github.com/llvm/llvm-project/blob/09662eeb46f76cda0a8d1102d8f67f7ad8ad64de/flang/include/flang/Lower/PFTBuilder.h#L145

Thanks for the patch. A few questions inline.

Thanks for this patch. A few comments inline.

We might even go further and generate these with TableGen. I guess we have most of the information in OMP.td or ACC.td for that.
Anyway might be wise to first land this patch and look at TableGen in a next step. Would be happy to look at it if it makes sense.

+1

LGTM.

Taking over from @DavidTruby on this patch. Contains the following modifications,
Added a yield terminator (omp.yield).
Restricted loop indices to llvm integer, integer, index types
Added an example
Addressed other minor text changes comments

This is a good verification check. A few questions.

Nit: I believe this only handles exit out of a region. It does not fully handle the branching (goto etc) out issue. Can you update the title and description to reflect this?

@thakis Can you revert the testcase fix in MLIR also which was necessary after this patch? https://reviews.llvm.org/rG0fc1aa22ee6ac337a5d51fa5666c9cd61da61b07

Updated mlir test also to include this change.
https://reviews.llvm.org/rG0fc1aa22ee6ac337a5d51fa5666c9cd61da61b07

I have added a fix to run the test only when the X86 target is available. Please feel free to change if it is not the correct fix.
https://github.com/llvm/llvm-project/commit/c551ba0e90bd2b49ef501d591f8362ba44e5484d

This change probably requires the X86 target.
// REQUIRES: x86-registered-target

LGTM. A few nit comments. Please check that check-clang also passes (since you changed a shared file, OMP.td).

LGTM.

LGTM.

This looks fine. Can all the tests be in a single file?

Thanks for this patch. A few comments inline.

Can you provide more information about the allocate directive in the summary/description of this patch?
-> Point to the section in the standard.
-> Explain that the standard specifies a declarative and executable version.

Thanks for this patch. Did you miss the array element check?

Thanks for this patch.

1. Does the EnumToString function in the parser work for this enum value?

File ./tools/mlir/include/mlir/Dialect/OpenMP/OpenMPOpsEnums.cpp.inc has the following function. Isn't it possible to use this function?
For e.g: If you pass it mlir::omp::ClauseProcBindKind::master it will return the string "master".

Thanks @praveen for this patch. Can you add a few positive tests as well?

I guess @clementval or @rriddle can approve the new extra functions added based on its need in Flang parse-tree lowering or on similar usage for StrEnumAttributes in rest of MLIR.