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

[HIP] Check for null arg to urKernelSetArgMemObj #1357

Merged
merged 1 commit into from
Apr 11, 2024

Conversation

EwanC
Copy link
Contributor

@EwanC EwanC commented Feb 19, 2024

The pProperties parameter to urKernelSetArgMemObj is optional, and so may be nullptr. However the HIP adapter assumes it is set currently. Updated this to check for nullptr, and if so assume that the buffer is read/write.

Updated CTS match file so that the test executable doesn't segfault early (CTS passes nullptr properties) and more tests run but fail. Discovered during testing of #1254 and is required for those CTS tests to pass.

DPC++ PR - intel/llvm#12758

@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (78ef1ca) 14.82% compared to head (40845b3) 12.72%.
Report is 34 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    #1357      +/-   ##
==========================================
- Coverage   14.82%   12.72%   -2.10%     
==========================================
  Files         250      238      -12     
  Lines       36220    35346     -874     
  Branches     4094     4010      -84     
==========================================
- Hits         5369     4498     -871     
- Misses      30800    30844      +44     
+ Partials       51        4      -47     

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

@EwanC EwanC force-pushed the ewan/hip_nullptr branch 2 times, most recently from b4d1e82 to 40845b3 Compare February 19, 2024 14:08
@EwanC EwanC marked this pull request as ready for review February 19, 2024 15:07
@EwanC EwanC requested review from a team as code owners February 19, 2024 15:07
@EwanC EwanC requested a review from hdelan February 19, 2024 15:07
Copy link
Contributor

@hdelan hdelan left a comment

Choose a reason for hiding this comment

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

LGTM

@EwanC EwanC added the ready to merge Added to PR's which are ready to merge label Feb 19, 2024
@kbenzie kbenzie added conformance Conformance test suite issues. hip HIP adapter specific issues labels Apr 3, 2024
@EwanC EwanC force-pushed the ewan/hip_nullptr branch 2 times, most recently from d1e4a86 to 49ba55d Compare April 11, 2024 12:40
The Properties parameter to `urKernelSetArgMemObj` is optional,
and so may be `nullptr`. However the HIP adapter assumes it is set
currently.

Updated this to check for nullptr, and if so assume that the buffer
is read/write.

Updated CTS match file so that the test executable doesn't segfault
early (CTS passes nullptr properties) and more tests run but fail.
@kbenzie kbenzie merged commit 760eaa3 into oneapi-src:main Apr 11, 2024
53 checks passed
kbenzie pushed a commit to reble/llvm that referenced this pull request Apr 11, 2024
Bump UR commit to include a bugfix for HIP UR adapter
dereferencing a nullptr oneapi-src/unified-runtime#1357
martygrant pushed a commit to intel/llvm that referenced this pull request Apr 12, 2024
Bump UR commit to include a bugfix for HIP UR adapter dereferencing a
nullptr oneapi-src/unified-runtime#1357
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
conformance Conformance test suite issues. hip HIP adapter specific issues ready to merge Added to PR's which are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants