Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SYCL][XPTI] Improvements to allow framework/app software level layers to provide code locations for sycl generated XPTI events. #15190

Merged
merged 11 commits into from
Oct 7, 2024

Conversation

gzadica1
Copy link
Contributor

It is useful for framework software layers which uses sycl in their
implementation (like IPEX) to provide framework level code location
information for XPTI events generated by sycl. This allows a framework
specific instrumentation tool to capture sycl XPTI events with code location
information coming from the framework level.
This allows the specific instrumentation tool, for example, to capture sycl
stream task_begin and task_end events and correlate the specific execution
with the upper layer graph node (or application level name of work) that this
task represents by querying the payload attached to the events.

The change does not require any new APIs or ABI change, to capture a
code location the framework software layer should instantiate the existing
sycl::detail::tls_code_loc_t object before calling a sycl entry point (usually
queue.submit or graph.add).

There are 3 commits in this PR:

  1. Change all sycl entry points that tries to set code location in TLS to use the
    code location that is already set in TLS, if one is set. Instead of passing on
    the entry point code location at any case.
  2. Payload for kernel execution commands uses the kernel name in place of the
    function name from code location. This changes this behavior in case that the
    upper layer software has captured code location in TLS before calling sycl.
  3. Fixes XPTI events in graph mode, some events were missing when bypassing scheduler.

It is useful for framework software layers which uses sycl in their
implementation (like IPEX) to provide framework level code location
information for XPTI events generated by sycl. This allows a
framework specific instrumentation tool to capture sycl xpti events
with code location information coming from the framework level.

This commit change all sycl entry points which tries to capture code
location in TLS to query the TLS and use the code location that was
already set in the TLS (if any) rather then using the code location
tried to be set.

framework level software can set code location in TLS by instatiating
the existing sycl::detail::tls_code_loc_t object.

Signed-off-by: Guy Zadicario <guy.zadicario@intel.com>
…payload.

When user has captured a code location in TLS before calling queue.submit,
use the captured code location function name if exist instead the kernel name
for the command group event payload. Kernel name is still added as metadata,
only the payload changes.

If user did not capture code location in TLS then use the kernel name for
backward compatability. Additional version for queue.submit_impl functions
added in order to propegate this state without breaking ABI.

Signed-off-by: Guy Zadicario <guy.zadicario@intel.com>
When graph is finalized kernels can be directly enqueued to a command
buffer without using the scheduler, in this case XPTI events for the
node_create/task_begin/task_end were missing.

Save code location from tls when CGF is added to a graph through
graph.add API. This captures user stored code location in TLS, if one
was set before calling graph.add

Signed-off-by: Guy Zadicario <guy.zadicario@intel.com>
Added small unittest case to check that user code location
is captured in node_create event:
NodeCreation.QueueParallelForWithUserCodeLoc

Signed-off-by: Guy Zadicario <guy.zadicario@intel.com>
@gzadica1 gzadica1 requested a review from EwanC September 6, 2024 09:02
@sergey-semenov
Copy link
Contributor

The changes look alright to me, but I'm not as familiar with XPTI. @KseniyaTikhomirova @tovinkere Could you please have a look?

Copy link
Contributor

@tovinkere tovinkere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@cppchedy cppchedy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - sycl/include/sycl/ext/oneapi/bindless_images.hpp.

Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just some nitpicks.

Please run clang-format to pass the pre-commit check.

sycl/include/sycl/detail/common.hpp Outdated Show resolved Hide resolved
sycl/source/detail/scheduler/commands.cpp Outdated Show resolved Hide resolved
sycl/source/detail/scheduler/commands.cpp Outdated Show resolved Hide resolved
sycl/source/detail/scheduler/commands.cpp Outdated Show resolved Hide resolved
sycl/source/detail/queue_impl.cpp Outdated Show resolved Hide resolved
sycl/unittests/xpti_trace/NodeCreation.cpp Outdated Show resolved Hide resolved
gzadica1 and others added 2 commits September 19, 2024 17:14
Co-authored-by: Sergey Semenov <sergey.semenov@intel.com>
Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There were some ABI compatibility test failures, please update sycl_symbols_linux.dump.

sycl/include/sycl/handler.hpp Outdated Show resolved Hide resolved
- moved handler::MIsTopCodeLoc to handler_impl
- updated new symbols in sycl_symbols_{linux,windows}.dump
Copy link
Contributor

@sergey-semenov sergey-semenov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one minor comment.

sycl/include/sycl/handler.hpp Show resolved Hide resolved
@sergey-semenov sergey-semenov merged commit ea95271 into intel:sycl Oct 7, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants