[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGda8bec47ab8c: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging (authored by jhuber6). Changed prior to commit: https://reviews.l

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jdoerfert accepted this revision. jdoerfert added a comment. Still LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://reviews.llvm.org/D87946 ___ cfe-commits mailing list cfe-commits@l

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 306210. jhuber6 added a comment. Rebasing Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://reviews.llvm.org/D87946 Files: clang/lib/CodeGen/CGOpenMPRuntime.cpp clang/test/OpenMP/capturing_i

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 301167. jhuber6 added a comment. Updating after D90172 and D89802 landed. I don't see it failing the tests anymore but I'll look into it more. I should probably make a test for the source l

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 added a comment. In D87946#2325934 , @grokos wrote: > In D87946#2325756 , @jhuber6 wrote: > >> Current build, fails `offloading/target_depend_nowait` for an unknown reason >> after calling cuStreamSynchroni

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

grokos added a comment. In D87946#2325756 , @jhuber6 wrote: > 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

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 297661. jhuber6 added a subscriber: tianshilei1992. jhuber6 added a comment. Current build, fails `offloading/target_depend_nowait` for an unknown reason after calling cuStreamSynchronize in __tgt_target_teams_mapper_nowait. Repository: rG LLVM Github Mon

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 297297. jhuber6 added a comment. Fixing tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://reviews.llvm.org/D87946 Files: clang/lib/CodeGen/CGOpenMPRuntime.cpp clang/test/OpenMP/captur

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 297003. jhuber6 added a subscriber: ABataev. jhuber6 added a comment. 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 dec

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 reopened this revision. jhuber6 added a comment. This revision is now accepted and ready to land. Closed accidentally, had the wrong revision link in another patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://reviews.ll

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

This revision was automatically updated to reflect the committed changes. Closed by commit rGd564409946a5: [OpenMP] Change CMake Configuration to Build for Highest CUDA Architecture by… (authored by jhuber6). Herald added a subscriber: mgorny. Changed prior to commit: https://reviews.llvm.org/D

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 295120. jhuber6 added a comment. Adding check for Windows file path. Updating some files after rebasing. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://reviews.llvm.org/D87946 Files: clang/lib/CodeGen/CGOpenMPRuntime.cpp clang

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

grokos added inline comments. Comment at: openmp/libomptarget/include/Ident.h:48-51 +auto removePath = [](const std::string &path) { +std::size_t pos = path.rfind('/'); +return path.substr(pos + 1); +}; jhuber6 wrote: > This will probably

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 added a comment. Should we wait until the next OpenMP LLVM meeting to push this? Comment at: openmp/libomptarget/include/Ident.h:48-51 +auto removePath = [](const std::string &path) { +std::size_t pos = path.rfind('/'); +return path.substr(pos + 1);

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 294352. jhuber6 added a comment. Adding message to build with debugging symbols if source location is not found. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://reviews.llvm.org/D87946 Files:

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. One minor remark from me, otherwise LGTM. @grokos Any concerns or is this OK? Comment at: openmp/libomptarget/src/interface.cpp:73-76 + FATAL_MESSAGE0(1, "fail

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 294296. jhuber6 added a comment. 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. Repository: rG

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 293748. jhuber6 added a comment. Changing legacy nowait calls to use the new function interface. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://reviews.llvm.org/D87946 Files: clang/lib/Code

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jdoerfert added inline comments. Comment at: openmp/libomptarget/src/interface.cpp:161 if (depNum + noAliasDepNum > 0) __kmpc_omp_taskwait(NULL, __kmpc_global_thread_num(NULL)); Remove this and call the nowait_mapper_loc version. Let's not duplicate log

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 293725. jhuber6 added a comment. Fixed not passing mappers to new functions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://reviews.llvm.org/D87946 Files: clang/lib/CodeGen/CGOpenMPRuntime.

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jdoerfert added inline comments. Comment at: openmp/libomptarget/src/interface.cpp:118 + __tgt_target_data_begin_mapper_loc(nullptr, device_id, arg_num, args_base, args, + arg_sizes, arg_types, nullptr); +} why not pass arg_mappers Comme

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 added a comment. In D87946#2289959 , @grokos wrote: > 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

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

grokos added a comment. In D87946#2287744 , @jhuber6 wrote: > 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 som

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 293550. jhuber6 added a comment. 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

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 added a comment. In D87946#2286434 , @grokos wrote: > 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 vers

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

grokos added a comment. In D87946#2286413 , @jhuber6 wrote: > 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` point

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 added a comment. In D87946#2286406 , @grokos wrote: > In D87946#2286024 , @jhuber6 wrote: > >> Added ident_t structs to additional runtime functions. > > Why are we adding the extra parameter to those additi

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

grokos added a comment. In D87946#2286024 , @jhuber6 wrote: > 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 t

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 added a comment. 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 clutte

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:446 +/// from +/// https://github.com/llvm/llvm-project/blob/master/openmp/runtime/src/kmp.h enum OpenMPLocationFlags : unsigned { A lot here seems unrelated. Did you clang format

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 293238. jhuber6 added a comment. Added ident_t structs to additional runtime functions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://reviews.llvm.org/D87946 Files: clang/lib/CodeGen/CGOpe

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 updated this revision to Diff 293220. jhuber6 added a comment. Herald added subscribers: aaron.ballman, hiraditya. Fixed failing tests from OpenMPOpt. Formatted files which results in a lot of changes showing. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://rev

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 added a comment. In D87946#2283049 , @jdoerfert wrote: > 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 ap

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jdoerfert added a comment. I guess you can try to use `sed` to update the tests. Comment at: openmp/libomptarget/include/omptarget.h:4 -// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. -// See https://llvm.org/LICENSE.txt for license information

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 added a comment. WIP for adding ident_t support to libomptarget. Still breaks some tests, just wanted to get a start. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D87946/new/ https://reviews.llvm.org/D87946 ___

[PATCH] D87946: [OpenMP] Add Location Fields to Libomptarget Runtime for Debugging

jhuber6 created this revision. jhuber6 added a reviewer: jdoerfert. jhuber6 added a project: OpenMP. Herald added subscribers: llvm-commits, openmp-commits, cfe-commits, jfb, guansong, yaxunl. Herald added projects: clang, LLVM. jhuber6 requested review of this revision. Herald added a subscriber: