Skip to content

[CUDA] Refactor device initialization #1363

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 2 commits into from
Mar 27, 2024

Conversation

hdelan
Copy link
Contributor

@hdelan hdelan commented Feb 19, 2024

Some logic for device initialization was split across platform init and device init. This duplicated some calls to get info for max block dim in Y and Z dimension. This rectifies that and makes sure the max block dims are only queried once, which is called from the device constructor.

@hdelan hdelan requested a review from a team as a code owner February 19, 2024 16:01
@hdelan hdelan force-pushed the refactor-device-initialization branch 2 times, most recently from 14d3701 to fe5d5d0 Compare February 19, 2024 17:52
@codecov-commenter
Copy link

codecov-commenter commented Feb 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 12.43%. Comparing base (78ef1ca) to head (2968cc1).
Report is 188 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    #1363      +/-   ##
==========================================
- Coverage   14.82%   12.43%   -2.40%     
==========================================
  Files         250      241       -9     
  Lines       36220    36242      +22     
  Branches     4094     4111      +17     
==========================================
- Hits         5369     4506     -863     
- Misses      30800    31732     +932     
+ Partials       51        4      -47     

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

@hdelan
Copy link
Contributor Author

hdelan commented Feb 23, 2024

Friendly ping @konradkusiak97

@konradkusiak97
Copy link
Contributor

Should there be an intel/llvm CI for this?

@hdelan
Copy link
Contributor Author

hdelan commented Feb 23, 2024

Oops forgot to link it. Here it is intel/llvm#12762

Copy link
Contributor

@konradkusiak97 konradkusiak97 left a comment

Choose a reason for hiding this comment

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

Aha that's strange how it was done before, it could've been partially my fault for not seeing this duplication with my caching PRs. Nice refactor, LGTM!

Just a side note, it seems some of those changes could also be applied to the HIP adapter, do you think it would be feasible to squeeze it together here?

@hdelan
Copy link
Contributor Author

hdelan commented Feb 23, 2024

Just a side note, it seems some of those changes could also be applied to the HIP adapter, do you think it would be feasible to squeeze it together here?

The HIP adapter is only checking this stuff once. It does it in a different manner, ie it doesnt use the ur helper, but I think there isn't anything wrong with the current approach of the HIP adapter so I would be inclined to leave it as it is. Let me know if you agree!

@hdelan hdelan added the ready to merge Added to PR's which are ready to merge label Feb 23, 2024
@konradkusiak97
Copy link
Contributor

Just a side note, it seems some of those changes could also be applied to the HIP adapter, do you think it would be feasible to squeeze it together here?

The HIP adapter is only checking this stuff once. It does it in a different manner, ie it doesnt use the ur helper, but I think there isn't anything wrong with the current approach of the HIP adapter so I would be inclined to leave it as it is. Let me know if you agree!

Sounds good!

hdelan added 2 commits March 20, 2024 10:06
Some logic for device initialization was split across platform
init and device init. This duplicated some calls to get info
for max block dim in Y and Z dimension. This rectifies that
and makes sure the max block dims are only queried once, which
is called from the device constructor.
Remove duplicate/redundant member vars
@hdelan hdelan force-pushed the refactor-device-initialization branch from fe5d5d0 to 2968cc1 Compare March 20, 2024 10:32
@hdelan hdelan added the v0.9.x Include in the v0.9.x release label Mar 26, 2024
@aarongreig aarongreig merged commit ff276ab into oneapi-src:main Mar 27, 2024
kbenzie pushed a commit to kbenzie/unified-runtime that referenced this pull request Apr 8, 2024
…lization

[CUDA] Refactor device initialization
@kbenzie kbenzie mentioned this pull request Apr 16, 2024
19 tasks
kbenzie pushed a commit to kbenzie/intel-llvm that referenced this pull request Apr 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Added to PR's which are ready to merge v0.9.x Include in the v0.9.x release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants