Page MenuHomePhabricator

[OpenMP] Add Location Fields to Libomptarget Runtime for Debugging
AcceptedPublic

Authored by jhuber6 on Sep 18 2020, 2:35 PM.

Details

Summary

Add support for passing source locations to libomptarget runtime functions. This will allow the runtime system to give much more insightful error messages and debugging values.

Diff Detail

Unit TestsFailed

TimeTest
4,130 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.AddFiles
Note: Google Test filter = DirectoryWatcherTest.AddFiles [==========] Running 1 test from 1 test case.
3,930 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.DeleteFile
Note: Google Test filter = DirectoryWatcherTest.DeleteFile [==========] Running 1 test from 1 test case.
4,080 mswindows > Clang-Unit.DirectoryWatcher/_/DirectoryWatcherTests_exe::DirectoryWatcherTest.ModifyFile
Note: Google Test filter = DirectoryWatcherTest.ModifyFile [==========] Running 1 test from 1 test case.

Event Timeline

jhuber6 created this revision.Sep 18 2020, 2:35 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptSep 18 2020, 2:35 PM
jhuber6 requested review of this revision.Sep 18 2020, 2:35 PM

WIP for adding ident_t support to libomptarget. Still breaks some tests, just wanted to get a start.

I guess you can try to use sed to update the tests.

openmp/libomptarget/include/omptarget.h
4

this got messed up

124

just make the type opaque for now: struct ident_t; We will include the original definition later.

I guess you can try to use sed to update the tests.

That's what I did for all the clang tests, they all pass as far as I know and I can use the built clang to correctly build offloaded applications for CUDA at least. The tests that are problematic are the OpenMPOpt tests because I'm assuming they had a hard coded argument index somewhere into one of the runtime functions that needs to be incremented by one now.

I also didn't add the an ident_t argument to all of the runtime functions, only the ones that I could see being generated by CGOpenMPRuntime.cpp. I just made those functions pass in a nullptr if they call into one of the functions that requires an ident_t.

openmp/libomptarget/src/interface.cpp
159

I should probably remove const here so we can pass loc to __kmpc_omp_taskwait.

jhuber6 updated this revision to Diff 293220.Sep 21 2020, 11:36 AM

Fixed failing tests from OpenMPOpt. Formatted files which results in a lot of changes showing.

jhuber6 set the repository for this revision to rG LLVM Github Monorepo.Sep 21 2020, 12:38 PM
jhuber6 updated this revision to Diff 293238.Sep 21 2020, 1:24 PM

Added ident_t structs to additional runtime functions.

jdoerfert added inline comments.Sep 21 2020, 1:41 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
446

A lot here seems unrelated. Did you clang format the file? I use the clang format patch tool.

Yeah, I did clang-format since the last patch complained about some formatting in the linter. Didn't expect it to change quite so much. Also for some reason it won't let me quote your comment. Might've been better to ignore it until the final commit so it doesn't clutter up everything.

grokos added a subscriber: grokos.Sep 21 2020, 3:56 PM

Added ident_t structs to additional runtime functions.

Why are we adding the extra parameter to those additional functions? Non-mapper API functions have been deprecated, clang does not emit them anymore...

Added ident_t structs to additional runtime functions.

Why are we adding the extra parameter to those additional functions? Non-mapper API functions have been deprecated, clang does not emit them anymore...

I wasn't aware they were explicitly deprecated. If we're keeping around old interfaces for backwards compatibility I should also add in the old mapper functions without the ident_t pointer and call into the new functions with a nullptr.

I wasn't aware they were explicitly deprecated. If we're keeping around old interfaces for backwards compatibility I should also add in the old mapper functions without the ident_t pointer and call into the new functions with a nullptr.

Correct, all __tgt_target_* functions not ending in _mapper are part of the old interface and we are keeping them for compatibility with older versions of clang. These older clang versions do not emit the location pointer anyway, so this extra argument should be removed and each such function should call into its new API equivalent passing a nullptr.

We need the location pointer only for __tgt_target_*_mapper functions as well as __kmpc_push_target_tripcount - this is the current interface.

Correct, all __tgt_target_* functions not ending in _mapper are part of the old interface and we are keeping them for compatibility with older versions of clang. These older clang versions do not emit the location pointer anyway, so this extra argument should be removed and each such function should call into its new API equivalent passing a nullptr.

We need the location pointer only for __tgt_target_*_mapper functions as well as __kmpc_push_target_tripcount - this is the current interface.

Since the functions are extern "C" I'll need to rename them to something else to keep them backwards compatible and then change what current Clang generates. something like __tgt_target_mapper_loc? Seems like a hacky solution to just keep adding suffixed whenever we want a new interface though.

jhuber6 updated this revision to Diff 293550.Sep 22 2020, 12:25 PM

Updated runtime library to have legacy calls into the new functions with source location information. Because the library interface requires C function naming this required adding a suffix to all the functions clang generates. Now tgt_*_mapper becomes tgt_*_mapper_loc. It's not a great solution but it's required if we want to keep this backwards compatible.

Seems like a hacky solution to just keep adding suffixed whenever we want a new interface though.

Yes, this used to be a point of contention within the community. We discussed the issue sometime ago and the majority of developers was in favor of this approach (as opposed to e.g. having an extra pointer to a structure which will contain additional information and which will be extended every time we add a new feature).

The libomptarget-part of this patch looks good, I'm leaving the other reviewers look at the clang-part.

Yes, this used to be a point of contention within the community. We discussed the issue sometime ago and the majority of developers was in favor of this approach (as opposed to e.g. having an extra pointer to a structure which will contain additional information and which will be extended every time we add a new feature).

The libomptarget-part of this patch looks good, I'm leaving the other reviewers look at the clang-part.

In the future we'll want the ability to pass the mapped variable's string name so we can refer to target pointers by name and line number in the runtime. Since this will require adding another argument containing a list of the names we'll probably want to try to add both at the same time to avoid the need for more suffixes.

jdoerfert added inline comments.Sep 23 2020, 7:03 AM
openmp/libomptarget/src/interface.cpp
128

why not pass arg_mappers

167

Please call the new version with a nullptr here.

194

Why is this passing nullptr in the end?

238

use the new version

289

use the new version

313

why not pass arg_mappers

367

Where is the non-loc version? why not next to it?

410

Please call the new version

jhuber6 updated this revision to Diff 293725.Sep 23 2020, 7:18 AM

Fixed not passing mappers to new functions.

jdoerfert added inline comments.Sep 23 2020, 7:55 AM
openmp/libomptarget/src/interface.cpp
164

Remove this and call the nowait_mapper_loc version. Let's not duplicate logic, also below a few times.

jhuber6 updated this revision to Diff 293748.Sep 23 2020, 8:20 AM

Changing legacy nowait calls to use the new function interface.

jhuber6 updated this revision to Diff 294296.Fri, Sep 25, 6:30 AM

Added definition for the ident_t struct from kmp.h along with a method to extract the source location information. Checking the target outcome now prints the file location if ident_t location is available.

jdoerfert accepted this revision.Fri, Sep 25, 9:21 AM

One minor remark from me, otherwise LGTM. @grokos Any concerns or is this OK?

openmp/libomptarget/src/interface.cpp
84–87

also check if info has an actual location please. If line and column are 0, print a message that -g can help provide source location information.

This revision is now accepted and ready to land.Fri, Sep 25, 9:21 AM
jhuber6 updated this revision to Diff 294352.Fri, Sep 25, 9:41 AM

Adding message to build with debugging symbols if source location is not found.

Should we wait until the next OpenMP LLVM meeting to push this?

openmp/libomptarget/include/Ident.h
49–52

This will probably break with a Windows file path, but I don't think you can even build most offloading if you're on Windows. Should I just add a processor check?

#ifdef _WIN32
#define PATH_DELIMITER '\\'
#else
#define PATH_DELIMITER '/'
#endif
grokos added inline comments.Tue, Sep 29, 5:37 AM
openmp/libomptarget/include/Ident.h
49–52

You are right, libomptarget does not run on Windows, but some fork which stays in sync with upstream libomptarget may do so and this change may break it. Can you add the proposed pre-processor check?

jhuber6 updated this revision to Diff 295120.Tue, Sep 29, 2:14 PM

Adding check for Windows file path. Updating some files after rebasing.

jhuber6 reopened this revision.Thu, Oct 8, 9:13 AM

Closed accidentally, had the wrong revision link in another patch.

This revision is now accepted and ready to land.Thu, Oct 8, 9:13 AM
jhuber6 updated this revision to Diff 297003.Thu, Oct 8, 10:00 AM
jhuber6 added a subscriber: ABataev.

Removing the _loc suffix. The Mapper API hasn't been officially released in Clang 11.x so we're still free to make changes. Currently working on augmenting the mapper API with variable declaration information so we can associate mapped pointers with their source names if the user specified debug locations.

jhuber6 updated this revision to Diff 297297.Fri, Oct 9, 12:14 PM

Fixing tests.

jhuber6 updated this revision to Diff 297661.Mon, Oct 12, 12:22 PM
jhuber6 added a subscriber: tianshilei1992.

Current build, fails offloading/target_depend_nowait for an unknown reason after calling cuStreamSynchronize in __tgt_target_teams_mapper_nowait.

Current build, fails offloading/target_depend_nowait for an unknown reason after calling cuStreamSynchronize in __tgt_target_teams_mapper_nowait.

Is your tree up to date? We had a problem with this test, which was fixed by D84470.

Current build, fails offloading/target_depend_nowait for an unknown reason after calling cuStreamSynchronize in __tgt_target_teams_mapper_nowait.

Is your tree up to date? We had a problem with this test, which was fixed by D84470.

I did a pull a few hours ago, so I have that patch in my tree. I compiled it both with master and this patch applied, it worked on master and failed using my patch. There are some slight differences between the code generated between the two in the device code generated, the host code only differs in the ident_t argument.

Master just has this under the device module

%class.omptarget_nvptx_Queue = type { [32 x %class.omptarget_nvptx_ThreadPrivateContext], [32 x %class.omptarget_nvptx_ThreadPrivateContext*], i32, [32 x i32], i32, [8 x i8] }

While this patch has this in addition.

%class.omptarget_nvptx_Queue = type { [32 x %class.omptarget_nvptx_ThreadPrivateContext.34], [32 x %class.omptarget_nvptx_ThreadPrivateContext.34*], i32, [32 x i32], i32, [8 x i8] }
%class.omptarget_nvptx_ThreadPrivateContext.34 = type { %class.omptarget_nvptx_TeamDescr.32, [1024 x %class.omptarget_nvptx_TaskDescr], [1024 x %class.omptarget_nvptx_TaskDescr*], %union.anon, [1024 x i32], [1024 x i64], [1024 x i64], [1024 x i64], [1024 x i64], i64, [8 x i8] }
%class.omptarget_nvptx_TeamDescr.32 = type { %class.omptarget_nvptx_TaskDescr, %class.omptarget_nvptx_WorkDescr, [32 x %struct.__kmpc_data_sharing_worker_slot_static] }

This probably has nothing to do with the problem but I'm wondering why this patch does anything that changes the device codegen.

jdenny added a subscriber: jdenny.Wed, Oct 14, 6:39 AM