Skip to content

Conversation

wenju-he
Copy link
Contributor

@wenju-he wenju-he commented Mar 2, 2024

Tested in intel/llvm#12965

@wenju-he wenju-he requested a review from a team as a code owner March 2, 2024 01:07
@wenju-he
Copy link
Contributor Author

wenju-he commented Mar 2, 2024

@kbenzie could you please review? thank you

@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.51%. Comparing base (78ef1ca) to head (2b3d016).
Report is 120 commits behind head on main.

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

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1406      +/-   ##
==========================================
- Coverage   14.82%   12.51%   -2.32%     
==========================================
  Files         250      239      -11     
  Lines       36220    35949     -271     
  Branches     4094     4076      -18     
==========================================
- Hits         5369     4498     -871     
- Misses      30800    31447     +647     
+ Partials       51        4      -47     

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

@isaacault
Copy link
Contributor

Is there a draft DPC++ PR for this as per the adapter change process?

@wenju-he
Copy link
Contributor Author

wenju-he commented Mar 5, 2024

Is there a draft DPC++ PR for this as per the adapter change process?

I'm afraid this is impossible since spirv part of device functions in https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/oneapi/bindless_images.hpp are not added yet. This means end-2-end test can't run on Intel DG2 GPU as the tests would fail in device code compilation stage.

@pbalcer
Copy link
Contributor

pbalcer commented Mar 5, 2024

Is there a draft DPC++ PR for this as per the adapter change process?

I'm afraid this is impossible since spirv part of device functions in https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/oneapi/bindless_images.hpp are not added yet. This means end-2-end test can't run on Intel DG2 GPU as the tests would fail in device code compilation stage.

We cannot merge changes into UR that would break upstream SYCL, since that would block updating UR in SYCL. Please indicate in the PR description that this requires other PRs to first be merged.

@wenju-he
Copy link
Contributor Author

wenju-he commented Mar 5, 2024

Is there a draft DPC++ PR for this as per the adapter change process?

I'm afraid this is impossible since spirv part of device functions in https://github.com/intel/llvm/blob/sycl/sycl/include/sycl/ext/oneapi/bindless_images.hpp are not added yet. This means end-2-end test can't run on Intel DG2 GPU as the tests would fail in device code compilation stage.

We cannot merge changes into UR that would break upstream SYCL, since that would block updating UR in SYCL. Please indicate in the PR description that this requires other PRs to first be merged.

This PR only fixes a coverity issue in UR.

Regarding intel/llvm repo this PR is a NFC change, because sycl bindless image tests in intel/llvm are not enabled on DG2 at all.

While we're waiting for dependent patch for sycl header that can enables end-2-end testing on DG2, we do have local version of the dependent patch for sycl headers, and the current support of bindless image in L0 adaptor enables us to debugging on DG2. The support is very helpful since it enables concurrent debugging from multiple people.

Copy link
Contributor

@isaacault isaacault left a comment

Choose a reason for hiding this comment

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

I see this is just a coverity fix - LGTM.

I'm aware the bindless images SPIR-V isn't added yet, and thus the DPC++ CI wouldn't hit this, making it NFC regardless. I'll leave it to the UR merge team to decide if we still need to follow the process.

@kbenzie kbenzie added the v0.9.x Include in the v0.9.x release label Mar 11, 2024
@kbenzie kbenzie force-pushed the urBindlessImagesSampledImageCreateExp-Coverity branch from d6c422c to 2b3d016 Compare March 11, 2024 10:56
kbenzie added a commit to kbenzie/intel-llvm that referenced this pull request Mar 11, 2024
@kbenzie kbenzie merged commit 93e8469 into oneapi-src:main Mar 12, 2024
sommerlukas pushed a commit to intel/llvm that referenced this pull request Mar 13, 2024
@wenju-he wenju-he deleted the urBindlessImagesSampledImageCreateExp-Coverity branch March 15, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.9.x Include in the v0.9.x release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants