This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Lower taskwait using OpenMP IR Builder
ClosedPublic

Authored by rogfer01 on Nov 4 2019, 3:18 PM.

Details

Summary

The code generation is exactly the same as it was.

But note that the special handling of untied tasks is still handled by emitUntiedSwitch in clang.

Diff Detail

Event Timeline

rogfer01 created this revision.Nov 4 2019, 3:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2019, 3:18 PM
ppenzin added a subscriber: ppenzin.Nov 4 2019, 4:19 PM

Also for taskwait inside untied tasks we should be reusing the available global_tid instead of calling __kmpc_global_thread_num again.

FWIW, calling getThreadID(getIdent(SrcLocStr)) is the right thing to do here. We do use a cache in that method and at the end of the optimization pipeline, after inlining, etc. we can replace/merge omp_get_thread_id, __kmpc_global_thread_num, and global_tid arguments as a separate (more powerful) step.

llvm/lib/IR/OpenMPIRBuilder.cpp
230 ↗(On Diff #227790)

I will modify my patch to allow an insertion point without block (in order to mimic Clang for now). Other than that in the emit vs. create thing, I think this is fine.

Nit: We both could actually cache getIdent(SrcLocStr) so we don't have to go through the map ;)

rogfer01 updated this revision to Diff 231106.Nov 26 2019, 11:03 AM
rogfer01 added a reviewer: kiranchandramohan.

ChangeLog:

  • Update patch against changes in D69785
rogfer01 updated this revision to Diff 231114.Nov 26 2019, 11:29 AM
rogfer01 marked an inline comment as done.

ChangeLog:

  • Enable the new code generation in codegen tests using taskwait

I think this is fine, one comment though.

llvm/include/llvm/Frontend/OpenMPIRBuilder.h
81 ↗(On Diff #231114)

If taskwait cannot cause control flow we don't need to return a new insert point, I think.

126 ↗(On Diff #231114)

Copy & paste, also see my other comment.

Also for taskwait inside untied tasks we should be reusing the available global_tid instead of calling __kmpc_global_thread_num again.

FWIW, We will just clean up in a later pass (under review). We have to do that anyway, adding some smarts here is barely worth it and I also removed similar logic from my patches.

jdoerfert accepted this revision.Nov 26 2019, 8:10 PM

LGTM, assuming the comments are addressed (incl. the commit message)

This revision is now accepted and ready to land.Nov 26 2019, 8:10 PM
rogfer01 edited the summary of this revision. (Show Details)Nov 27 2019, 2:28 AM

Also for taskwait inside untied tasks we should be reusing the available global_tid instead of calling __kmpc_global_thread_num again.

FWIW, We will just clean up in a later pass (under review). We have to do that anyway, adding some smarts here is barely worth it and I also removed similar logic from my patches.

Sure. I actually forgot to update the summary because you already mentioned this IIRC.

clang/lib/CodeGen/CGOpenMPRuntime.cpp
6347

This is to preserve exactly the same behaviour as before.

rogfer01 marked an inline comment as done.Nov 27 2019, 2:30 AM
rogfer01 added inline comments.
llvm/include/llvm/Frontend/OpenMPIRBuilder.h
81 ↗(On Diff #231114)

Ah I see, we want a similar behaviour to that of IRBuilder in which the new IP is updated after something has been emitted.

Makes sense.

rogfer01 retitled this revision from [WIP][OpenMP] Lower taskwait using OpenMP IR Builder to [OpenMP] Lower taskwait using OpenMP IR Builder.Nov 27 2019, 3:17 AM
rogfer01 edited the summary of this revision. (Show Details)
rogfer01 updated this revision to Diff 231207.Nov 27 2019, 3:30 AM

ChangeLog:

  • Remove unnecessary return of the insertion point and update code accordingly.
llvm/lib/Frontend/OpenMPIRBuilder.cpp
258 ↗(On Diff #231207)

Is the emit function required here? We could have this in the create function itself right?

rogfer01 updated this revision to Diff 233531.Dec 12 2019, 1:20 AM

ChangeLog:

  • Rebase
rogfer01 updated this revision to Diff 237861.Jan 14 2020, 12:11 AM
rogfer01 edited the summary of this revision. (Show Details)

ChangeLog:

  • Rebase
rogfer01 updated this revision to Diff 244578.Feb 14 2020, 12:48 AM

ChangeLog:

  • Rebase
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 14 2020, 1:56 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript