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.

Sign up for GitHub

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

Jump to bottom

[Bindless][CUDA] Unique addressing modes per dimension #1168

Merged
merged 5 commits into from
Jan 30, 2024
Merged

[Bindless][CUDA] Unique addressing modes per dimension #1168

merged 5 commits into from
Jan 30, 2024

Conversation

Seanst98
Copy link
Contributor

@Seanst98 Seanst98 commented Dec 7, 2023

Extends the CUDA adapter to allow for unique addressing modes per dimension

Modifies:

  • image.cpp
  • sampler.hpp
  • sampler.cpp

Corresponding intel-llvm PR: intel/llvm#12109

@Seanst98 Seanst98 requested review from a team as code owners December 7, 2023 13:57
@Seanst98 Seanst98 mentioned this pull request Dec 7, 2023
Merged
@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2023 •
edited
Loading

Codecov Report

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

Comparison is base (858225a) 15.46% compared to head (5fc4109) 15.42%.
Report is 3 commits behind head on main.

Files Patch % Lines
test/conformance/enqueue/urEnqueueKernelLaunch.cpp 0.00% 47 Missing ⚠️
test/conformance/enqueue/urEnqueueUSMMemcpy2D.cpp 0.00% 17 Missing ⚠️
test/conformance/enqueue/helpers.h 0.00% 13 Missing ⚠️
test/conformance/testing/source/utils.cpp 0.00% 10 Missing ⚠️
test/conformance/testing/include/uur/fixtures.h 0.00% 2 Missing ⚠️

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

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1168      +/-   ##
==========================================
- Coverage   15.46%   15.42%   -0.04%     
==========================================
  Files         238      238              
  Lines       33883    33956      +73     
  Branches     3747     3753       +6     
==========================================
  Hits         5239     5239              
- Misses      28593    28666      +73     
  Partials       51       51

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

@Seanst98
Copy link
Contributor Author

Pinging @oneapi-src/unified-runtime-cuda-write for approval and ready to merge tag.

ldrumm
ldrumm reviewed Jan 15, 2024
View reviewed changes
Copy link
Contributor

@ldrumm ldrumm left a comment

Choose a reason for hiding this comment

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

Why are modified files listed in the commit message?

@Seanst98 Seanst98 force-pushed the sean/unique-addr-mode-per-dim-adapters branch from 153d878 to 7b0a45a Compare January 15, 2024 15:59
@Seanst98
Copy link
Contributor Author

Seanst98 commented Jan 15, 2024 •
edited
Loading

Why are modified files listed in the commit message?

I've removed them from the commit message, @ldrumm.

@Seanst98 Seanst98 force-pushed the sean/unique-addr-mode-per-dim-adapters branch from d683997 to a0555f7 Compare January 19, 2024 16:21
@Seanst98
Copy link
Contributor Author

Pinging @oneapi-src/unified-runtime-cuda-write for approvals/reviews and ready to merge tag.

@ldrumm ldrumm requested a review from GeorgeWeb January 23, 2024 11:44
GeorgeWeb
GeorgeWeb approved these changes Jan 23, 2024
View reviewed changes
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.

CUDA changes LGTM after connecting it with the changes from intel/llvm.

.. a minor comment but the changes look fine.

source/adapters/cuda/image.cpp
ur_sampler_addressing_mode_t AddrModeProp =
hSampler->getAddressingModeDim(i);
if (AddrModeProp == (UR_SAMPLER_ADDRESSING_MODE_CLAMP_TO_EDGE -
UR_SAMPLER_ADDRESSING_MODE_NONE)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to always substract UR_SAMPLER_ADDRESSING_MODE_NONE which is 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point. It's a carry-over from before UR where PI_SAMPLER_ADDRESSING_MODE_NONE was not 0.

Would you prefer I remove it?

@Seanst98 Seanst98 added the ready to merge Added to PR's which are ready to merge label Jan 26, 2024
@kbenzie kbenzie merged commit 3225b82 into oneapi-src:main Jan 30, 2024
52 checks passed
martygrant pushed a commit to intel/llvm that referenced this pull request Jan 31, 2024
@Seanst98 @kbenzie
[SYCL][Bindless] Unique sampler addressing modes per dimension (#12109)
b897152 
Add the ability to specify unique addressing modes per dimension to the
bindless_image_sampler

Corresponding CUDA adapter UR PR:
oneapi-src/unified-runtime#1168

---------

Co-authored-by: Kenneth Benzie (Benie) <k.benzie83@gmail.com>
@Seanst98 Seanst98 deleted the sean/unique-addr-mode-per-dim-adapters branch January 31, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldrumm ldrumm ldrumm left review comments

@isaacault isaacault isaacault approved these changes

@GeorgeWeb GeorgeWeb GeorgeWeb approved these changes

Assignees
No one assigned
Labels
ready to merge Added to PR's which are ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Seanst98 @codecov-commenter @GeorgeWeb @ldrumm @isaacault @kbenzie