This is an archive of the discontinued LLVM Phabricator instance.

[openmp] Add OMPT initialization in libomptarget
ClosedPublic

Authored by lechenyu on Apr 2 2021, 10:08 AM.

Details

Summary

When loading libomptarget, the init function in libomptarget/src/rtl.cpp
will search for the libomptarget_start_tool function using libdl.
libomptarget_start_tool will pass those OMPT callbacks related to target
constructs to libomptarget

Diff Detail

Event Timeline

lechenyu created this revision.Apr 2 2021, 10:08 AM
lechenyu requested review of this revision.Apr 2 2021, 10:08 AM
lechenyu updated this revision to Diff 335118.Apr 3 2021, 5:44 PM

Run clang-format to fix warnings in code style

grokos added a subscriber: grokos.Apr 13 2021, 10:49 AM
grokos added inline comments.
openmp/libomptarget/src/ompt-target.h
13

ompt_internal.h resides under runtime/src/, not libomptarget/src/.

openmp/libomptarget/src/rtl.cpp
71

If the call to start_tool failed, is it correct to claim that ompt is initialized? On the other hand, maybe it's enough that you've set ompt_target_enabled to 0.

protze.joachim added inline comments.Apr 14 2021, 1:33 AM
openmp/libomptarget/src/rtl.cpp
60

Below in line 103, __kmpc_get_target_offload is called directly without dlsym. Is that the preferred way to call into libomp?

71

The OMPT initialization mechanism should happen once during the execution of an application.
Is there a scenario, where this code might execute twice, so that the if(!ompt_initialized) might have any effect?

I just realized, that we will need to check what libomp is doing after forking a process, but that's a separate issue.

openmp/runtime/src/ompt-general.cpp
134

This function is called by libomptarget and should not be weak.

138–140

This will call __kmp_middle_initialize whenever libomptarget gets initialized. Is this ok?

lechenyu updated this revision to Diff 338233.Apr 16 2021, 2:19 PM
Update ompt initialization in libomptarget:
    1. Change the initialization function name to "libomp_start_tool"
    2. Libomptarget uses weak symbol rather than libdl to retrieve
       libomp_start_tool
    3. Libomp_start_tool only sets the bitmap, since libomptarget will
       delegate the callback invocation to libomp. Libomptarget no
       longer needs the addresses of OMPT callbacks and entry points
grokos added inline comments.Apr 16 2021, 3:26 PM
openmp/libomptarget/src/interface.cpp
312–314 ↗(On Diff #338233)

Remove

lechenyu updated this revision to Diff 338261.Apr 16 2021, 5:43 PM

Remove an unnecessary conditional directive

lechenyu added inline comments.Apr 16 2021, 5:46 PM
openmp/libomptarget/src/interface.cpp
312–314 ↗(On Diff #338233)

I have removed it in the new diff

openmp/libomptarget/src/ompt-target.h
13

This file no longer needs "ompt-internal.h" in the updated diff. For omp-tools.h, I have added LIBOMP_INCLUDE_DIR into LIBOMPTARGET_LLVM_INCLUDE_DIRS (at openmp/libomptarget/src/CMakeLists.txt:25).

jdenny added a subscriber: jdenny.Apr 21 2021, 8:20 AM

reverse ping.

Can we move this along? Are there blockers? If we need discussion let's do it next Wednesday.

lechenyu updated this revision to Diff 359353.Jul 16 2021, 9:16 AM

Remove redundant macro definitions in omp-tools.h.var and rebase to the llvm/main

lechenyu updated this revision to Diff 362438.Jul 28 2021, 10:09 AM
hbae added inline comments.Jul 29 2021, 10:53 AM
openmp/libomptarget/src/rtl.cpp
199

Can we also turn if off when !libomp_start_tool?

protze.joachim added inline comments.Jul 30 2021, 7:03 AM
openmp/libomptarget/src/rtl.cpp
199

Do you mean the ompt_initialized variable? I think, we just can remove this variable. LoadRTLs should only be called once during the execution.

All other code just looks at ompt_target_enabled, which is 0 if !libomp_start_tool.

hbae accepted this revision.Aug 2 2021, 1:28 PM
hbae added inline comments.
openmp/libomptarget/src/rtl.cpp
199

Looks OK for now.
We may be able to add any clean-up changes in the follow-up patches.

This revision is now accepted and ready to land.Aug 2 2021, 1:28 PM
lechenyu updated this revision to Diff 364110.Aug 4 2021, 8:04 AM

Remove the unnecessary variable "libomp_start_tool".

protze.joachim accepted this revision.Aug 4 2021, 8:12 AM

The fixed version LGTM

openmp/libomptarget/src/rtl.cpp
199

The ompt_initialized variable is gone now.

hbae accepted this revision.Aug 4 2021, 8:58 AM

LGTM

This revision was landed with ongoing or failed builds.Aug 4 2021, 9:00 AM
This revision was automatically updated to reflect the committed changes.