Support for taskwait nowait clause with placeholder for runtime changes.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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. |
clang/lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
5817 | I would recommend to make it a member of OMPTaskDataTy |
clang/lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
5817 | Yeah, that sounds better. |
In the latest revision that was submitted the following changes were made:
- Changed HaveNoTaskWaitClause to HasNowaitClause.
- HasNowaitClause is now a data member of OMPTaskDataTy.
- OpenMPVer is now a data member of OpenMPIRBuilder.
clang/lib/CodeGen/CGStmtOpenMP.cpp | ||
---|---|---|
5266 | Data.HasNowaitClause = S.hasClausesOfKind<OMPNowaitClause>(); |
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?
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; } |
The current patch has the following changes:
- "if" statement removed for Data.HasNowaitClause = S.hasClausesOfKind<OMPNowaitClause>();
- OpenMPVer replaced with OpenMPVersion.
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. |
Changes for the build failure due to git-clang-format. Some minor changes are also because of git pull.
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). |
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.
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.
Changes for the suggestion : "Always use kmpc_omp_taskwait_51 and eliminate the old kmpc_omp_taskwait".
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.
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?) |
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.)
(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.
clang/test/OpenMP/taskwait_depend_nowait_codegen.cpp | ||
---|---|---|
2 | Please do not add any new tests using -no-opaque-pointers. |
Taking care of :
- "The nowait clause may only appear on a taskwait directive if the depend clause is present.".
- "Please do not add any new tests using -no-opaque-pointers".
- Added a new test to expect failure for point 1.
Use CGF.Int32Ty