Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[openmp] Add OMPT initialization in libomptarget

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



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.

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


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

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


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.


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


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
    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
312–314 ↗(On Diff #338233)


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
312–314 ↗(On Diff #338233)

I have removed it in the new diff


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

Can we also turn if off when !libomp_start_tool?

protze.joachim added inline comments.Jul 30 2021, 7:03 AM

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.

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


The ompt_initialized variable is gone now.

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


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.