Page MenuHomePhabricator

[OpenMP] libomptarget: move debugging dumps under control of environment variable OMP_TARGET_DEBUG
ClosedPublic

Authored by sdmitriev on May 15 2017, 7:43 PM.

Details

Summary

Libomptarget currently dumps lots of debugging messages to stderr by default (in debug build) and the only way to turn debugging dumps off is rebuilding libomptarget in release mode which is not very convenient.

Attached patch disables default debugging dumps for libomptarget and plugins and moves dumps under control of environment variable OMP_TARGET_DEBUG=<integer>. Dumps are enabled when OMP_TARGET_DEBUG environment variable is set to a positive integer value.

Debugging dumps are available only in debug build; release build does not support it.

Diff Detail

Repository
rL LLVM

Event Timeline

sdmitriev created this revision.May 15 2017, 7:43 PM
grokos edited edge metadata.May 18 2017, 2:18 AM

Is there a reason why we have a separate int DebugLevel in every plugin source file and omptarget.cpp and parse OMP_TARGET_DEBUG from all those places instead of declaring extern int DebugLevel in omptarget.h, defining int DebugLevel in omptarget.cpp and parsing OMP_TARGET_DEBUG only from within RTLsTy::LoadRTLs()?

grokos added a project: Restricted Project.
grokos added a subscriber: openmp-commits.

I've added Jonas as a reviewer because I'm not very familiar with regression tests.

Jonas, can you review the changes under libomptarget/test?

Defining int DebugLevel in omptarget.cpp only would create a dependency between libomptarget.so and each plugin which I guess should be avoided. I assume we do not want to link plugins with libomptarget library.

tlwilmar requested changes to this revision.May 31 2017, 2:21 PM
tlwilmar added a subscriber: tlwilmar.

I don't think the environment variable should be named "OMP_TARGET_DEBUG" as that would suggest it is something coming from the OpenMP specification, which I don't believe it is. I'd suggest just keeping the LIBOMPTARGET_DEBUG name at all levels.

This revision now requires changes to proceed.May 31 2017, 2:21 PM

I can change the name for this environment variable it as you suggested, but it will make it consistent with other environment variable's name that is already used by libomptarget - OMP_TARGET_OFFLOAD (look at openmp/libomptarget/src/omptarget.cpp, line 285).

*inconsistent

Hmmm... that's disturbing.

The policy for libomp is, if it's in the spec, it starts with OMP_, and if it's just in our implementation, it starts with KMP_.
I guess this is not true for libomptarget. I'd like to get Ravi's opinion on this, so don't make a change yet.

Hahnfeld edited edge metadata.May 31 2017, 11:04 PM

I can change the name for this environment variable it as you suggested, but it will make it consistent with other environment variable's name that is already used by libomptarget - OMP_TARGET_OFFLOAD (look at openmp/libomptarget/src/omptarget.cpp, line 285).

I criticized that when the code went in. The convincing argument here was that OMP_NUM_TEAMS may be defined in a later version of the spec and OMP_TARGET_OFFLOAD was already used for some bots. As that's not yet true for OMP_TARGET_DEBUG, I would also prefer a different name. Ideally, we should also get rid of OMP_TARGET_OFFLOAD and rename it to something different...

sdmitriev updated this revision to Diff 101770.Jun 7 2017, 10:33 AM
sdmitriev edited edge metadata.

I have changed the name of environment variable which controls debugging dumps to LIBOMPTARGET_DEBUG.

Just to share what I have locally. I have OFFLOAD_DEBUG=1 for the plugin side debug messages, and DEVICE_DEBUG=1 for the deviceRTL side messages.

Just to share what I have locally. I have OFFLOAD_DEBUG=1 for the plugin side debug messages, and DEVICE_DEBUG=1 for the deviceRTL side messages.

So are you suggesting that each plugin has its own env variable to turn debug output on/off?

Just to share what I have locally. I have OFFLOAD_DEBUG=1 for the plugin side debug messages, and DEVICE_DEBUG=1 for the deviceRTL side messages.

So are you suggesting that each plugin has its own env variable to turn debug output on/off?

No. I am thinking one should be OK for all plugins.

For LIBOMPTARGET_DEBUG, will it control the device side of the debug message later on?

You call it "target", that gives me the impression it will. But maybe that is not the case. And LIBOMPOFFLOAD_DEBUG may be a better name.

In my scenario, I do have two flags, one is for the host (actually the code is almost identical to yours), and one for the device.

The device side of the variable may need to be specific to each device. The one I have can work for GPU, but it may not work well for PHI. As in this case the device and host will share the same openmp runtime mechanism?

Just to share what I have locally. I have OFFLOAD_DEBUG=1 for the plugin side debug messages, and DEVICE_DEBUG=1 for the deviceRTL side messages.

So are you suggesting that each plugin has its own env variable to turn debug output on/off?

No. I am thinking one should be OK for all plugins.

For LIBOMPTARGET_DEBUG, will it control the device side of the debug message later on?

Libomptarget itself does not have a device side part yet, but if we decide to add it in future I think it would be consistent to control all messages coming from it by LIBOMPTARGET_DEBUG setting.

You call it "target", that gives me the impression it will. But maybe that is not the case. And LIBOMPOFFLOAD_DEBUG may be a better name.

No, the reason why it is named this way is actually different – it controls debugging messages from the libomptarget library, so it looks quite natural to call it LIBOMPTARGET_DEBUG.

In my scenario, I do have two flags, one is for the host (actually the code is almost identical to yours), and one for the device.

The device side of the variable may need to be specific to each device. The one I have can work for GPU, but it may not work well for PHI. As in this case the device and host will share the same openmp runtime mechanism?

I guess you are talking about the device side RTL which is not directly related to libomptarget and plugins, right? If that is the case then I think we cannot restrict device libraries to use whatever they want. It should be fine to have device specific settings for each particular target.

LIBOMPTARGET_DEBUG is intended to control debugging messages for libomptarget and plugins only. Everything else is outside of its control.

I don't think the environment variable should be named "OMP_TARGET_DEBUG" as that would suggest it is something coming from the OpenMP specification, which I don't believe it is. I'd suggest just keeping the LIBOMPTARGET_DEBUG name at all levels.

Terry, I have changed the environment variable name as you suggested. Do you have more comments for this patch?

I don't think the environment variable should be named "OMP_TARGET_DEBUG" as that would suggest it is something coming from the OpenMP specification, which I don't believe it is. I'd suggest just keeping the LIBOMPTARGET_DEBUG name at all levels.

Terry, I have changed the environment variable name as you suggested. Do you have more comments for this patch?

No more comments. Thanks!

If we reached an agreement on the environment variable name and there are no more comments for this change, can it be approved?

Hahnfeld accepted this revision.Aug 1 2017, 11:38 PM

Given the lack of additional comments, LGTM. Please adapt the commit description to reflect the changes during review.

This revision was automatically updated to reflect the committed changes.