This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] [OMPT] [3/8] Implemented callback registration in libomptarget
ClosedPublic

Authored by dhruvachak on Apr 18 2022, 7:24 PM.

Details

Summary

The purpose of this patch is to have tool-provided callbacks registered
in libomptarget. The overall design document is in
https://rice.app.box.com/s/pf3gix2hs4d4o1aatwir1set05xmjljc

Defined a class OmptDeviceCallbacksTy that will be used by libomptarget
and a plugin for callbacks registered by a tool. Once the callbacks are
registered in libomp, a lookup function is passed to libomptarget that is
used to retrieve the callbacks and register them in libomptarget.

Patch from John Mellor-Crummey <johnmc@rice.edu>
(With contributions from Dhruva Chakrabarti <Dhruva.Chakrabarti@amd.com>)

Diff Detail

Event Timeline

dhruvachak created this revision.Apr 18 2022, 7:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2022, 7:24 PM
dhruvachak requested review of this revision.Apr 18 2022, 7:24 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 18 2022, 7:24 PM
dhruvachak retitled this revision from [OpenMP] [OMPT] Implemented callback registration in libomptarget to [OpenMP] [OMPT] [3/8] Implemented callback registration in libomptarget.Jun 20 2022, 7:44 PM
dhruvachak edited the summary of this revision. (Show Details)
dhruvachak edited the summary of this revision. (Show Details)Oct 31 2022, 2:48 PM

Rebased. Please review.

jplehr added a subscriber: jplehr.Dec 1 2022, 7:56 AM
jplehr added a comment.Dec 1 2022, 8:10 AM

My comment about the naming applies to all objects / variables etc.
I'm unsure whether it would help readability, but I have in mind that it makes the external/user-facing API clearly distinguishable from the internal one.

openmp/libomptarget/include/ompt_device_callbacks.h
60

I wonder if we should limit the use of snake_case to only the OMP/OMPT interface functions and types, and stick with the LLVM naming conventions otherwise?

Variable name changed.

dhruvachak marked an inline comment as done.Dec 1 2022, 11:29 AM
dhruvachak added inline comments.
openmp/libomptarget/include/ompt_device_callbacks.h
60

Thanks for the comment. Changed.

jplehr accepted this revision.Dec 1 2022, 12:08 PM
This revision is now accepted and ready to land.Dec 1 2022, 12:08 PM
dhruvachak marked an inline comment as done.Dec 1 2022, 12:15 PM

Anyone else has any comment before I land this patch?

Woohoo!

John Mellor-Crummey Professor
Dept of Computer Science Rice University
email: johnmc@rice.edu phone: 713-348-5179

Looks like this patch breaks the build.

FAILED: libomptarget/src/CMakeFiles/omptarget.dir/ompt_callback.cpp.o
/home/ac.shilei.tian/Documents/build/llvm/release/bin/clang++ -DGTEST_HAS_RTTI=0 -DOMPTARGET_DEBUG -DOMPT_SUPPORT=1 -I/home/ac.shilei.tian/Documents/vscode/llvm-project/llvm/include -I/home/ac.shilei.tian/Documents/build/llvm/release/include -I/home/ac.shilei.tian/Documents/build/openmp/release/runtime/src -I/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include -Wall -Wcast-qual -Wformat-pedantic -Wimplicit-fallthrough -Wsign-compare -Wno-enum-constexpr-conversion -Wno-extra -Wno-pedantic -std=c++17 -O3 -DNDEBUG -fPIC  -fno-exceptions -fno-rtti -MD -MT libomptarget/src/CMakeFiles/omptarget.dir/ompt_callback.cpp.o -MF libomptarget/src/CMakeFiles/omptarget.dir/ompt_callback.cpp.o.d -o libomptarget/src/CMakeFiles/omptarget.dir/ompt_callback.cpp.o -c /home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/src/ompt_callback.cpp
In file included from /home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/src/ompt_callback.cpp:22:
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
    FOREACH_OMPT_TARGET_CALLBACK(OmptBindCallback);
                                 ^
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
/home/ac.shilei.tian/Documents/vscode/llvm-project/openmp/libomptarget/include/ompt_device_callbacks.h:45:34: error: use of undeclared identifier 'DEBUG_PREFIX'
12 errors generated.

Looks like this patch breaks the build.

I did not see this problem in my build. I will investigate. For now, I reverted the commit.

I can repro the failure with -DLIBOMPTARGET_ENABLE_DEBUG=ON added. I will update the patch.

dhruvachak reopened this revision.Dec 1 2022, 11:30 PM
This revision is now accepted and ready to land.Dec 1 2022, 11:30 PM

Fixed build failure with LIBOMPTARGET_ENABLE_DEBUG.

@tianshilei1992 I tested builds w/ and w/o -DLIBOMPTARGET_ENABLE_DEBUG=ON. Any concern if I land the updated patch?

tianshilei1992 accepted this revision.Dec 7 2022, 6:34 PM

Please go ahead land it if it is clear on your side.