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

[EXP][CMDBUF] Support for Node Profiling #1309

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

mfrancepillois
Copy link
Contributor

  • Adds a new entry point urEventGetSyncPointProfilingInfoExp. This function queries the profiling information of a sync-point. It takes the handle of the event returned from the graph submission and the sync-point associated with the node from which we want to obtain the profiling information.
  • Implements node profiling support.

Adds a new entry point urEventGetSyncPointProfilingInfoExp. This function queries the profiling information of a sync-point. It takes the handle of the event returned from the graph submission and the sync-point associated with the node from which we want to obtain the profiling information.
Implements node profiling support.
@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2024

Codecov Report

Attention: 124 lines in your changes are missing coverage. Please review.

Comparison is base (78ef1ca) 14.82% compared to head (c805e6d) 12.82%.
Report is 11 commits behind head on main.

Files Patch % Lines
include/ur_print.hpp 35.00% 26 Missing ⚠️
source/loader/layers/validation/ur_valddi.cpp 43.75% 18 Missing ⚠️
...e/common/umf_pools/disjoint_pool_config_parser.cpp 0.00% 15 Missing ⚠️
source/loader/ur_ldrddi.cpp 53.57% 13 Missing ⚠️
source/loader/layers/tracing/ur_trcddi.cpp 53.84% 12 Missing ⚠️
source/adapters/null/ur_nullddi.cpp 40.00% 9 Missing ⚠️
source/common/umf_helpers.hpp 0.00% 9 Missing ⚠️
source/loader/ur_libapi.cpp 0.00% 8 Missing ⚠️
source/common/logger/ur_logger_details.hpp 61.53% 5 Missing ⚠️
test/usm/usmPoolManager.cpp 0.00% 5 Missing ⚠️
... and 1 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1309      +/-   ##
==========================================
- Coverage   14.82%   12.82%   -2.01%     
==========================================
  Files         250      238      -12     
  Lines       36220    35479     -741     
  Branches     4094     4032      -62     
==========================================
- Hits         5369     4549     -820     
- Misses      30800    30926     +126     
+ Partials       51        4      -47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mfrancepillois
Copy link
Contributor Author

Linked DPC++ PR: intel/llvm#12592

@mfrancepillois mfrancepillois marked this pull request as ready for review February 2, 2024 17:21
@mfrancepillois mfrancepillois requested review from a team as code owners February 2, 2024 17:21
scripts/core/exp-command-buffer.yml Outdated Show resolved Hide resolved
scripts/core/exp-command-buffer.yml Outdated Show resolved Hide resolved
source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved
source/adapters/level_zero/command_buffer.cpp Outdated Show resolved Hide resolved
mfrancepillois and others added 3 commits February 8, 2024 11:45
Co-authored-by: Ewan Crawford <ewan.cr@gmail.com>
Co-authored-by: Ewan Crawford <ewan.cr@gmail.com>
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

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

Minor outstanding comment about parameter naming, but LGTM otherwise

Copy link
Contributor

@GeorgeWeb GeorgeWeb left a comment

Choose a reason for hiding this comment

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

Just a couple of nitpicks otherwise the CUDA and HIP adapter change look good.

*pDdiTable ///< [in,out] pointer to table of DDI function pointers
) {
auto retVal = validateProcInputs(version, pDdiTable);
if (UR_RESULT_SUCCESS != retVal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Prefer regular lhs_val == const_rhs_val, i.e. retVal != UR_RESULT_SUCCESS over the Yoda notation, for readability reasons. This is not a blocker.

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'm a little confused by your comments (this one and following), because all the conditions in these files use Yoda notation.

*pDdiTable ///< [in,out] pointer to table of DDI function pointers
) {
auto retVal = validateProcInputs(version, pDdiTable);
if (UR_RESULT_SUCCESS != retVal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the other case wrt the Yoda notation used for the comparsion.

*pDdiTable ///< [in,out] pointer to table of DDI function pointers
) {
auto retVal = validateProcInputs(version, pDdiTable);
if (UR_RESULT_SUCCESS != retVal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure of the style used in this adapter, can comment just about the CUDA and HIP ones, but same nit-pick comment wrt Yoda notation used for the comparison. Again, nothing blocking though.

*pDdiTable ///< [in,out] pointer to table of DDI function pointers
) {
auto retVal = validateProcInputs(version, pDdiTable);
if (UR_RESULT_SUCCESS != retVal) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: more Yoda notation.

@mfrancepillois
Copy link
Contributor Author

@oneapi-src/unified-runtime-level-zero-write could you please review this PR?

Copy link
Contributor

@martygrant martygrant 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

@PietroGhg PietroGhg left a comment

Choose a reason for hiding this comment

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

Native CPU lgtm, thank you

Copy link
Contributor

@nrspruit nrspruit left a comment

Choose a reason for hiding this comment

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

LGTM for level zero

@mfrancepillois mfrancepillois added the ready to merge Added to PR's which are ready to merge label Feb 20, 2024
@EwanC EwanC removed the ready to merge Added to PR's which are ready to merge label Apr 2, 2024
@kbenzie kbenzie added loader Loader related feature/bug specification Changes or additions to the specification level-zero L0 adapter specific issues cuda CUDA adapter specific issues hip HIP adapter specific issues opencl OpenCL adapter specific issues native-cpu Native CPU adapter specific issues labels Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda CUDA adapter specific issues hip HIP adapter specific issues level-zero L0 adapter specific issues loader Loader related feature/bug native-cpu Native CPU adapter specific issues opencl OpenCL adapter specific issues specification Changes or additions to the specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants