This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Begin Printing Information Dumps In Libomptarget and Plugins
ClosedPublic

Authored by jhuber6 on Sep 4 2020, 2:25 PM.

Details

Summary

This patch starts adding support for adding information dumps to libomptarget and rtl plugins. The information printing is controlled by the LIBOMPTARGET_INFO environment variable introduced in D86483. The goal of this patch is to provide the user with additional information about the device during kernel execution and providing the user with information dumps in the case of failure. This patch added the ability to dump the pointer mapping table as well as printing the number of blocks and threads in the cuda RTL

Diff Detail

Event Timeline

jhuber6 created this revision.Sep 4 2020, 2:25 PM
jhuber6 requested review of this revision.Sep 4 2020, 2:25 PM
jhuber6 updated this revision to Diff 290030.Sep 4 2020, 2:31 PM
ye-luo requested changes to this revision.Sep 4 2020, 2:32 PM
ye-luo added a subscriber: ye-luo.
ye-luo added inline comments.
openmp/libomptarget/include/Debug.h
107

Please document all the macros, in this file, like functions. Say a bit about their intended use case.

openmp/libomptarget/src/interface.cpp
28

Update comments

This revision now requires changes to proceed.Sep 4 2020, 2:32 PM
jhuber6 updated this revision to Diff 290033.Sep 4 2020, 2:44 PM

Added additional comments. Should I add them to the doxygen notes at the top?

ye-luo added a comment.Sep 4 2020, 2:58 PM

Added additional comments. Should I add them to the doxygen notes at the top?

@jdoerfert Any suggestions?

I think the current documentation is insufficient. Adding/updating any documents for any of the macro or overall are helpful.
Although other macros are not touched, it is still helpful to document them like those you added. You are probably the best person who knows how and when such macros should be used now.

Added additional comments. Should I add them to the doxygen notes at the top?

@jdoerfert Any suggestions?

I think the current documentation is insufficient. Adding/updating any documents for any of the macro or overall are helpful.
Although other macros are not touched, it is still helpful to document them like those you added. You are probably the best person who knows how and when such macros should be used now.

I guess I'd prefer doxygen above the macro, as if they are functions. The reason is that I want us to make them functions at some point anyway.

openmp/libomptarget/plugins/cuda/src/rtl.cpp
499

Maybe: "Device %d supports up to % CUDA blocks and %d threads with a warp size of %d".
Also, why don't we make info behave like DP in debug mode and remove the DP macro?

948

Can we please merge those two messages:

INFO("Launching kernel with %d blocks and %d threads in %s mode\n", CudaBlocksPerGrid,
         CudaThreadsPerBlock,(KernelInfo->ExecutionMode == SPMD)? "SPMD" : "Generic")
openmp/libomptarget/src/interface.cpp
37

Don't we have to add space here to match the 18 above?

jhuber6 updated this revision to Diff 290044.Sep 4 2020, 3:34 PM

Updated messages and comments.

jhuber6 added inline comments.Sep 4 2020, 3:35 PM
openmp/libomptarget/src/interface.cpp
37

The width of the pointer is 16 plus the "0x" at the start.

I'm fine with this, one minor comment below. @ye-luo wdyt?

openmp/libomptarget/plugins/cuda/src/rtl.cpp
934

Forgot, we need to print the device number here too. Also the kernel name if we have it. That name is not great but has the line number in it (IIRC).

Maybe add a first mandatory argument to the INFO macro that is the device number. Then we can always forward something like: "[OMP-INFO][Device %d] " __VAR_ARGS__.
I see we already have the first part (to some degree) adding the device part would be good (I think).

jhuber6 added inline comments.Sep 4 2020, 4:07 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
934

You mean using the CUDA RTL to get the name from the CUfunction value in the KernelTy struct?

jdoerfert added inline comments.Sep 4 2020, 4:20 PM
openmp/libomptarget/plugins/cuda/src/rtl.cpp
934

We add it to KernelTy. When we do KernelsList.emplace_back(Func, ExecModeVal); in rtl.cpp, we know the name.

jhuber6 updated this revision to Diff 290490.Sep 8 2020, 8:15 AM

Added the target kernel name and device ID to the message. Added a new method to get the offload entry given the pointer. I can't decide if it's better to just always print the device ID if we have it versus putting it in the macro itself.

Does this require any test files?

Added the target kernel name and device ID to the message. Added a new method to get the offload entry given the pointer. I can't decide if it's better to just always print the device ID if we have it versus putting it in the macro itself.

I believe there is no reason to be somewhere in this methods without a device ID. Thus, I would require it as part of the function/macro.

Does this require any test files?

I think that would be good. We should be able to set the INFO variable in the run line as well. Do we have a test for the DP macro we can copy?

openmp/libomptarget/plugins/cuda/src/rtl.cpp
299

can we replace findOffloadEntry with this please.

ye-luo accepted this revision.EditedSep 8 2020, 8:27 AM

The changes I requested as been added. Remove my blocking. Still need other reviews to be addressed.

This revision is now accepted and ready to land.Sep 8 2020, 8:27 AM
jhuber6 updated this revision to Diff 290503.Sep 8 2020, 9:20 AM

Added small test file.

jhuber6 updated this revision to Diff 290747.Sep 9 2020, 8:33 AM

Changed table dumps to require LIBOMPTARGET_INFO > 1. We could move this to a bitvector in the future. Added an extra message to inform users to set LIBOMPTARGET_INFO on failure.

This revision was landed with ongoing or failed builds.Sep 9 2020, 9:18 AM
This revision was automatically updated to reflect the committed changes.
ye-luo added inline comments.Sep 10 2020, 6:23 AM
openmp/libomptarget/src/interface.cpp
81

Need to be more specific about the "tables"
host-device memory mapping tables?