Skip to content

[SYCL] Implement multiple tracing levels for SYCL_UR_TRACE #14983

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

Merged
merged 3 commits into from
Aug 15, 2024

Conversation

callumfare
Copy link
Contributor

This undoes some changes to tracing that were made when porting from PI to UR, and means that SYCL_UR_TRACE now has the same behavior as the SYCL_PI_TRACE option that it replaced.

Also only enable the UR tracing layer when SYCL_UR_TRACE is set to the appropriate level, rather than always enabling it when UR_LOG_TRACING is set - fixes #14926

@omarahmed1111
Copy link
Contributor

@intel/dpcpp-doc-reviewers @intel/dpcpp-esimd-reviewers @intel/dpcpp-tools-reviewers @intel/sycl-graphs-reviewers please review, Thanks!

Copy link
Contributor

@sarnex sarnex left a comment

Choose a reason for hiding this comment

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

lgtm minus a question

Comment on lines +242 to +246
| 2 | Enable tracing of the UR calls |
| -1 | Enable all levels of tracing |
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the difference between 2 and -1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2 enables just the tracing of UR calls, but -1 enables tracing of the UR calls as well as the basic device/adapter discovery tracing, i.e. 1 and 2 combined (and implicitly also any future tracing levels that may be added).

Copy link
Contributor

@sarnex sarnex Aug 13, 2024

Choose a reason for hiding this comment

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

thanks, think we can add that explanation to the doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the documentation now

@omarahmed1111
Copy link
Contributor

@intel/dpcpp-doc-reviewers Please review, Thanks!

This change means SYCL_UR_TRACE has the same behavior as the SYCL_PI_TRACE
option that it replaced.
@omarahmed1111
Copy link
Contributor

@intel/llvm-reviewers-runtime Please review, Thanks!

@aelovikov-intel
Copy link
Contributor

To be honest, it doesn't make sense to me to have any UR traces inside SYCL RT. Is that bad naming or some bigger issue with tracing design?

@dm-vodopyanov
Copy link
Contributor

@callumfare please follow https://github.com/intel/llvm/blob/sycl/sycl/doc/developer/ContributeToDPCPP.md#commit-message

@callumfare callumfare changed the title Implement multiple tracing levels for SYCL_UR_TRACE [SYCL] Implement multiple tracing levels for SYCL_UR_TRACE Aug 14, 2024
@kbenzie
Copy link
Contributor

kbenzie commented Aug 14, 2024

To be honest, it doesn't make sense to me to have any UR traces inside SYCL RT. Is that bad naming or some bigger issue with tracing design?

This is restoring the behavior of SYCL_PI_TRACE. It arguably shouldn't have been used in all the places it was either.

@callumfare
Copy link
Contributor Author

To be honest, it doesn't make sense to me to have any UR traces inside SYCL RT. Is that bad naming or some bigger issue with tracing design?

@aelovikov-intel The tracing is implemented in UR, but the environment variables to enable it are quite verbose:
UR_ENABLE_LAYERS=UR_LAYER_TRACING UR_LOG_TRACING=level:info;output:stdout;flush:info UR_LOG_LOADER=level:info;output:stdout;flush:info

The SYCL_UR_TRACE option is just a shortcut for this, and also provides some continuity from SYCL_PI_TRACE which was recently removed

@aelovikov-intel
Copy link
Contributor

This is restoring the behavior of SYCL_PI_TRACE. It arguably shouldn't have been used in all the places it was either.

But previously plugins were part of this project, so while not ideal implementation-wise, still not leaking abstractions terribly. I don't see that being true now that UR is separate.

@aelovikov-intel
Copy link
Contributor

The SYCL_UR_TRACE option is just a shortcut for this, and also provides some continuity from SYCL_PI_TRACE which was recently removed

Why can't that shortcut be provided at the UR side?

@callumfare
Copy link
Contributor Author

Why can't that shortcut be provided at the UR side?

@aelovikov-intel It could, although from a SYCL user's perspective it makes sense for the option to start with the SYCL_ prefix, and having an option with that name in UR wouldn't be ideal. Also as Benie said above, some elements of the tracing are implemented in SYCL-RT, which does mean that the option has a misleading name, so something should probably change.

I'd rather get this merged asap to get rid of functional differences with SYCL_PI_TRACE, and then we could revisit the design of the tracing in future.

Copy link
Contributor

@aelovikov-intel aelovikov-intel left a comment

Choose a reason for hiding this comment

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

Not a fan of it, but if it's needed to unblock folks...

@aelovikov-intel aelovikov-intel merged commit 390a472 into intel:sycl Aug 15, 2024
13 checks passed
AlexeySachkov pushed a commit to AlexeySachkov/llvm that referenced this pull request Nov 26, 2024
This undoes some changes to tracing that were made when porting from PI
to UR, and means that `SYCL_UR_TRACE` now has the same behavior as the
`SYCL_PI_TRACE` option that it replaced.

Also only enable the UR tracing layer when `SYCL_UR_TRACE` is set to the
appropriate level, rather than always enabling it when `UR_LOG_TRACING`
is set - fixes intel#14926
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.

implement UR_LOG_TRACING env var in UR instead of sycl RT, currently it's implemented in ur.hpp - first raised in this comment
8 participants