This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Clang Support for taskwait nowait clause
ClosedPublic

Authored by koops on Aug 12 2022, 11:23 PM.

Diff Detail

Event Timeline

koops created this revision.Aug 12 2022, 11:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2022, 11:23 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
koops requested review of this revision.Aug 12 2022, 11:23 PM
Herald added a project: Restricted Project. · View Herald Transcript
koops updated this revision to Diff 452734.Aug 15 2022, 10:25 AM
koops edited the summary of this revision. (Show Details)
koops updated this revision to Diff 452924.Aug 16 2022, 2:16 AM
tianshilei1992 added inline comments.Aug 30 2022, 9:27 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
5817

better to use consistent name as declaration. how about HasNowaitClause?

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
634 ↗(On Diff #452924)

I'd keep the style consistent. Here there is a default argument bool HaveNoTaskWaitClause = false but no for emitTaskwaitCall. It's better to use default argument (potentially save a couple of changes) for both, or no default for both.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1232

Is the OpenMPVer necessary as an argument? Do we have alternative way to tell the version? If not, I think it makes more sense to take it as a data member of OpenMPIRBuilder instead of passing as a function argument.

ABataev added inline comments.Aug 30 2022, 9:32 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
5817

I would recommend to make it a member of OMPTaskDataTy

tianshilei1992 added inline comments.Aug 30 2022, 9:33 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
5817

Yeah, that sounds better.

koops updated this revision to Diff 462237.Sep 22 2022, 10:51 AM
koops edited the summary of this revision. (Show Details)
koops edited the summary of this revision. (Show Details)Sep 22 2022, 10:56 AM

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.
ABataev added inline comments.Sep 22 2022, 11:01 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
5266

Data.HasNowaitClause = S.hasClausesOfKind<OMPNowaitClause>();

koops updated this revision to Diff 462397.EditedSep 22 2022, 11:57 PM

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>();

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

ABataev added inline comments.Sep 29 2022, 3:08 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
5266–5267

No need for if here, just

Data.HasNowaitClause = S.hasClausesOfKind<OMPNowaitClause>();
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
1418 ↗(On Diff #462397)

int OpenMPVersion = 0?

1421 ↗(On Diff #462397)

int getOpenMPVersion() const {return OpenMPVersion; }
Plus, do you really need an OpenMP version check here? Just call the new function always

koops updated this revision to Diff 464144.Sep 29 2022, 11:13 PM

The current patch has the following changes:

  1. "if" statement removed for Data.HasNowaitClause = S.hasClausesOfKind<OMPNowaitClause>();
  2. OpenMPVer replaced with OpenMPVersion.
koops added a comment.EditedSep 29 2022, 11:31 PM

Reply

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
1421 ↗(On Diff #462397)

The new function __kmpc_omp_taskwait_deps_51, in openmp/runtime/src/kmp_taskdeps.cpp, is a placeholder for taking into account "nowait" clause from Spec 5.1. The scheduling of tasks might change during runtime due to "nowait" clause. Hence a check for OpenMP version is needed.

koops updated this revision to Diff 469120.Oct 19 2022, 11:29 PM

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

jdoerfert added inline comments.Oct 20 2022, 6:42 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
5873

Same as above.

5878

Can we avoid the duplication, always make the array 7 long, and use the ArrayRef substring capability to pass in either 6 or 7 elements?

Conceptually something like this:

Fill all
emitRuntimeCall(..., ArrayRef(DepWaitTaskArgs, OpenMP >= 51 ? 7: 6)

On second thought, why do we need to ever emit the old runtime call at all? Always emit the new one.

llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
80 ↗(On Diff #469120)

Not sure a default is wise here. Users should pick. And we might want to have a "latest" constant somewhere, just set to 99.

643 ↗(On Diff #469120)

The clause is not NoTaskwait, it's nowait, right? Why not HasNowaitClause?

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
1250

Same as above, new version only is sufficient.

2370

Unrelated and not even sure which one we should use nowadays. Please split this off.

openmp/runtime/src/kmp.h
4015

No default arguments here please.

openmp/runtime/src/kmp_taskdeps.cpp
905

Implement the old version by calling the new version with no_wait false. I assume you copied the original code, this should have indicated a problem (wrt. code duplication).

koops added a comment.EditedOct 21 2022, 4:40 AM

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.

Thank you for your detailed comments.

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.

My comments are not about avoiding kmpc_omp_taskwait_51 in the runtime. On the contrary. My comments say: Always use kmpc_omp_taskwait_51 and eliminate the old kmpc_omp_taskwait. Do not generate calls to kmpc_omp_taskwait but to kmpc_omp_taskwait_51 instead. Implement kmpc_omp_taskwait with a call to kmpc_omp_taskwait_51. Doing both will remove most of the new code and we still get the same benefits.
I hope this is clearer now.

jdoerfert retitled this revision from Clang Support for taskwait nowait clause to [OpenMP] Clang Support for taskwait nowait clause.Oct 21 2022, 8:49 AM
koops updated this revision to Diff 473869.Nov 7 2022, 9:11 PM

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

Herald added a reviewer: dcaballe. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
koops added a comment.Nov 14 2022, 9:10 PM

Can someone please review my latest changes?

ABataev added inline comments.Nov 15 2022, 3:46 AM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
4769

Use CGF.Int32Ty

5840

Use CGF.Int32Ty

koops updated this revision to Diff 475499.Nov 15 2022, 9:10 AM

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

This revision is now accepted and ready to land.Nov 15 2022, 9:16 AM
koops updated this revision to Diff 478083.Nov 27 2022, 3:51 AM

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

koops updated this revision to Diff 481302.Dec 8 2022, 8:48 AM

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

Do you need to commit it?

koops added a comment.Dec 8 2022, 10:15 AM

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.

Ok, I'll commit it.

This revision was landed with ongoing or failed builds.Dec 8 2022, 12:44 PM
This revision was automatically updated to reflect the committed changes.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 8 2022, 12:44 PM
mstorsjo added inline comments.
openmp/runtime/src/dllexports
349

This is missing a similar export of __kmpc_omp_taskwait_51, which causes a bunch of tests to fail if running them on Windows now. (Also, where do the numbers for these exports get assigned?)

cchen reopened this revision.Dec 9 2022, 12:31 PM
This revision is now accepted and ready to land.Dec 9 2022, 12:31 PM
koops updated this revision to Diff 481740.Dec 9 2022, 1:45 PM

Addition of __kmpc_omp_taskwait_51 in dllexports.

koops added a comment.EditedDec 9 2022, 1:57 PM

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?

koops updated this revision to Diff 481742.Dec 9 2022, 2:36 PM

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?

Thanks, now this seems to work fine for me! (There seems to be another unrelated regression at the same time though, regarding the symbol __kmpc_fork_call_if.)

koops added a comment.Dec 10 2022, 8:38 AM

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

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

Yeah, that one should be separate, I guess it stems from D138495.

This updated patch should be ok with me to recommit.

Why do we need __kmpc_omp_taskwait_51 ? Taskwait nowait only makes sense with dependencies. It should only result in additional nodes/edges in the task dependency graph without any code to execute. There is not taskwait (aka task barrier) in this case.

The taskwait construct has the restriction in the spec:

The nowait clause may only appear on a taskwait directive if the depend clause is present.

nikic added a subscriber: nikic.Dec 12 2022, 8:03 AM
nikic added inline comments.
clang/test/OpenMP/taskwait_depend_nowait_codegen.cpp
2

Please do not add any new tests using -no-opaque-pointers.

cchen requested changes to this revision.Dec 12 2022, 8:18 AM
This revision now requires changes to proceed.Dec 12 2022, 8:18 AM
koops updated this revision to Diff 484290.Dec 20 2022, 8:38 AM

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.
cchen accepted this revision.Dec 20 2022, 10:12 AM
This revision is now accepted and ready to land.Dec 20 2022, 10:12 AM
This revision was landed with ongoing or failed builds.Dec 20 2022, 10:14 AM
This revision was automatically updated to reflect the committed changes.