Can someone please review the changes that I uploaded last week?

Using isOpenMPWorksharingDirective( ) for the "omp loop bind(parallel)" and "omp loop bind(teams)".
Added extra tests in loop_bind_messages.cpp.

Can someone please check for MacOS? Yesterday when this support was committed, the CHECK statements in tests loop_bind_codegen.cpp and loop_bind_enclosed.cpp had failed on Mac and I received a comment as follows:

When relanding, please remember to put a link to the review in the commit message.

I have made the CHECK statements generic enough to match those on Mac with CHECK for the functions that the test were expected.

Pulling in latest changes.
The patch uploaded 2 hours back was a wrong one (meant for another feature) and has been unrolled back with this patch.

Making the CHECK pattern generic to match platforms tested after committing changes.

Hello Nico Weber,

In clang/test/OpenMP/loop_bind_enclosed.cpp Generalizing the CHECK pattern for aarch64-linux, s390x-linux and ppc64le-linux.

1. Support for PCH Serilization/deserilization for the PrevMapLoopConstruct variable in OMPExecutableDirective.
2. Modification of clang/test/PCH/pragma-loop.cpp to include "#pragma omp loop bind" with different parameters for bind clause.

1. clang/test/OpenMP/loop_bind_enclosed.cpp : Converting main and function names within it to use regular expressions to accomodate windows platform.

i) Instead of calling setMappedDirective() after the the creation of the Directive, made the MappedDirective a parameter of the Create() method.
ii) setMappedDirective() is called from the Directive::Create()
iii) The inheritance is : OMPExecutableDirective --> OMPLoopBasedDirective --> OMPLoopDirective --> OMPSimdDirective, from OMPSimdDirective constructor it involves a lot of changes to initialize PrevMappedDirective variable in OMPExecutableDirective starting from OMPSimdDirective, even if the variable is "protected". Hence we call setMappedDirective() from OMPSimdDirective::Create().

1. clang-format error in clang/include/clang/AST/StmtOpenMP.h corrected.
2. Correcting clang/test/OpenMP/loop_bind_enclosed.cpp to use regular expressions for names of functions to accomodate names generated on Microsoft Windows.

1. Converting prevMapped to PrevMappedDirective
2. Reducing the CHECK patterns in loop_bind_enclosed.cpp to the most essential ones to succeed matching for the IR generation on windows.

Correction for clang-format errors.

Fixing Alexey's suggestions.

Moving the variable MappedDirective into structure SharingMapTy. Enough comments have been put in place to explain.
Modified some of the loop_bind tests to cater to windows and debian platform.

1. Removing the size for the template constructor, llvm::SmallVectorImpl<OMPClause *> &ClausesWithoutBind.
2. In the diagnostic message of err_omp_bind_required_on_loop, using single quotes for the name 'loop' construct.
3. Minor changes to the tests clang/test/OpenMP/loop_bind_codegen.cpp & clang/test/OpenMP/loop_bind_enclosed.cpp to use a generic pattern matching expression for the names & reducing some checks.

Addressing Alexey's comments.
The name matching in loop_bind_messages.cpp & generic_loop_codegen.cpp tests changed to take care of the failures.

1. Taking care of Alexy & David suggestions: a) Using update_cc_test_checks.py to generate CHECK statements. b) Change in error message from "handled" to "allowed". c) Adding comments for the bind clause. d) Mangled names of the functions in the CHECK statements are more generic with regular expressions.

1. Taken care of Alexy's comments.
2. Reverting back changes to StmtPrinter.cpp because state of AST needs to be shown as is to the developer of clang when -ast-print/-ast-dump is used. This also meant some changes to the test cases to reflect the Mapped directives (from "omp loop" to "omp for", "omp distribute" or "omp simd").

1. Addition of extra test case loop_bind_enclosed.cpp.
2. Changing of name from Map1D to MappedDirective

However, generic_loop_messages.cpp & generic_loop_ast_print.cpp are present and provide a good coverage.

I rather doubt that these tests provide good coverage, since you're changing the directive kind here on the fly. This is a very new functionality, which was not tested before.
Add the tests for nesting of the regions, i.e. loop bind directive, enclosed in different regions, and diagnostics.

1. Taken care of some of the comments by Alexey.
2. Added extra tests of loop_bind_messages.cpp

However, generic_loop_messages.cpp & generic_loop_ast_print.cpp are present and provide a good coverage.

1. Adding semantic test clang/test/OpenMP/loop_bind_messages.cpp.
2. Changes suggested by Alexey.
3. >Why need to drop bind clause here? The new Directives to which loop directive is being mapped to, do not contain the bind clause. (a) omp loop bind(parallel) ----> omp for (b) omp loop bind(teams) -----> omp distribute (c) omp loop bind(thread) ------> omp simd

Removing changes from :

1. clang/include/clang/AST/StmtOpenMP.h
2. clang/lib/AST/StmtOpenMP.cpp
3. clang/include/clang/Parse/Parser.h

These were useful when the code was in CodeGen to handle the bind clause.

Moving the code to SemaOpenMP.cpp from CodeGen.

1. formatting
2. Adding lit test
3. Removing bind clause from the set of clauses passed during bind(parallel) to the OMPForDirective and bind(teams) to the OMPDistributeDirective.

bind(thread) is not working at present. I have uploaded this patch to obtain feedback mainly on this.

In D141541#4052062, @jdoerfert wrote:

In D141541#4050383, @koops wrote:

nowait clause on taskwait was marked as "part:done" because I have implemented the support for the clang compiler. The implementation for the runtime needs to be done and I have created a place holder for it. https://reviews.llvm.org/D131830 had the subject line " [OpenMP] Clang Support for taskwait nowait clause " .

So, we accept it but handle it conservatively, e.g.. ignore the nowait but act on the dependences?

nowait clause on taskwait was marked as "part:done" because I have implemented the support for the clang compiler. The implementation for the runtime needs to be done and I have created a place holder for it. https://reviews.llvm.org/D131830 had the subject line " [OpenMP] Clang Support for taskwait nowait clause " .

Taking care of :

1. "The nowait clause may only appear on a taskwait directive if the depend clause is present.".
2. "Please do not add any new tests using -no-opaque-pointers".
3. Added a new test to expect failure for point 1.

(There seems to be another unrelated regression at the same time though, regarding the symbol __kmpc_fork_call_if.)

I do not know about this regression. I have not touched this function.

git pull & reloading the earlier patch.

Martin Storsjö (or anybody who works on windows) I have uploaded a new patch which has __kmpc_omp_taskwait_51 in dllexports. Can you please check the patch on windows?

Addition of __kmpc_omp_taskwait_51 in dllexports.

Yes, I don't have commit rights. I have tried to reproduce the pre-merge check failures on RedHat and Ubuntu but, the tests don't fail.

No changes. Pulling in recent changes to trigger a rebuild.

No Changes. Pulling in recent changes to trigger a rebuild.

Changes for "Use CGF.Int32Ty" instead of CGF.IntTy.

Can someone please review my latest changes?

Changes for the suggestion : "Always use kmpc_omp_taskwait_51 and eliminate the old kmpc_omp_taskwait".

Johannes Doerfert,
Most of your comments are related to avoiding having the kmpc_omp_taskwait_51( ) , in the runtime, and instead introduce a "nowait" related parameter in the kmpc_omp_taskwait( ) itself. I have explained earlier that kmpc_omp_taskwait_51( ) is a placeholder for introducing new code, at a later stage. The scheduling of tasks might change in the runtime due to "nowait" clause. I have done this with the sole purpose of having backward compatibility. If you feel that is not needed then I will remove the new code and modify the Code Generation also according to your comments.

Changes for the build failure due to git-clang-format. Some minor changes are also because of git pull.

Reply

The current patch has the following changes:

1. "if" statement removed for Data.HasNowaitClause = S.hasClausesOfKind<OMPNowaitClause>();
2. OpenMPVer replaced with OpenMPVersion.

I seem to have made changes for all the feedback which were provided. Can someone please look at the changes I have uploaded?

Contains changes suggested for:

Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5215
+ if (S.hasClausesOfKind<OMPNowaitClause>()) Data.HasNowaitClause = true;
Replaced with:
Data.HasNowaitClause = S.hasClausesOfKind<OMPNowaitClause>();

In the latest revision that was submitted the following changes were made:

1. Changed HaveNoTaskWaitClause to HasNowaitClause.
2. HasNowaitClause is now a data member of OMPTaskDataTy.
3. OpenMPVer is now a data member of OpenMPIRBuilder.

Changes suggested by aaron.ballman to avoid failures on windows.

Hello Kevin,

In D123286#3527374, @koops wrote:

In D123286#3513797, @tianshilei1992 wrote:

Can we have test for right usage?

I do not understand "test for right usage". From the specifications the only right usage for "omp nothing" will be in the metadirective. If you have meeting notes when "omp nothing" was framed please share it.

Clang formatting for variables in ParseOpenMP.cpp

Fixing a minor error : clang formatting of variable names to avoid build errors.

In D123286#3513797, @tianshilei1992 wrote:

Can we have test for right usage?

changes to support clang format.

Further changes in the code to confirm to the clang format.

I have tried on x64 RH and x64 SuSe. I could not reproduce the failures seen on x64 debian. https://reviews.llvm.org/D118550 also has similar failures on x64 debian. There is a comment " I think the test failures are spurious (but not 100% sure)" So, are these failures pre-existing before the changes in the current support for "atomic compare fail: Parser & Support" were done?

Took care of the clang format.

1. changes in flang/lib/Semantics/check-omp-structure.cpp to avoid build failure
2. prevent "requires" directive test from failing.

Can I please have the detailed log for the build failures? I do not have the necessary setup to test those builds.