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

Extend api #268

Merged
merged 11 commits into from
May 2, 2024
Merged

Extend api #268

merged 11 commits into from
May 2, 2024

Conversation

csbnw
Copy link
Contributor

@csbnw csbnw commented May 1, 2024

Description

Extend the API so that cudawrappers can also be used in PMT:

  • Added cu::Device::getUUID()
  • Added cudawrappers::nvml target with only the functionality that PMT needs

Related issues:

  • N.A.

Instructions to review the pull request

  • Check that CHANGELOG.md has been updated if necessary

@csbnw csbnw added the enhancement New feature or request label May 1, 2024
@csbnw csbnw requested a review from matmanc May 1, 2024 14:11
@csbnw csbnw self-assigned this May 1, 2024
matmanc
matmanc reviewed May 1, 2024
View reviewed changes
include/cudawrappers/nvml.hpp Outdated Show resolved Hide resolved
include/cudawrappers/nvml.hpp Outdated Show resolved Hide resolved
@csbnw
Copy link
Contributor Author

csbnw commented May 1, 2024

After a discussion with @matmanc, we concluded that it is potentially dangerous if the nvml::Device initializes NVML and especially destroys it again (when other instances may still be active). A new nvml::Context class is added to guard against this. A nvml::Context must now be created first and next passed to the nvml::Device constructor. We also discussed adding Context::getDevice, but I think the current solution is cleaner.

matmanc
matmanc approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@matmanc matmanc left a comment

Choose a reason for hiding this comment

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

looks good

@csbnw csbnw merged commit 6a2902f into main May 2, 2024
7 checks passed
@csbnw csbnw deleted the extend-api branch May 2, 2024 07:02
loostrum added a commit that referenced this pull request Jun 18, 2024
@loostrum @john-romein
@csbnw @pre-commit-ci
Merge main into hipified (#282)
6ef7d19 
* Added cu::Function::occupancyMaxActiveBlocksPerMultiprocessor()

* Let target_embed_source inline local header files (#263)

* Fix working directory for linking source into object file (#264)

* Update readthedocs configuration (#265)

* Fix target_embed_source when input file is a relative path (#266)

* Removed deprecated cu::Context::setSharedMemConfig (#267)

* Extend api (#268)

* Add Device::getUUID
* Add initial nvml wrapper

* Fix target_embed_source (#269)

* Fix target embed source paths (#270)

* Fix path given to ld in target_embed_source
* Fix missing arg in string replace

* Fix CI (#271)

* Add runtime subpackage
* Fix symbols for test_nvrtc

* [pre-commit.ci] pre-commit autoupdate (#259)

* Update clang-format: v17.0.6 → v18.1.4
* Replace cmakelang with clang-format and cmake-format

* Add nvrtc::findIncludePath (#272)

* Make nvrtc::findIncludePath inline (#273)

* Fix string replacements (#275)

* Fix inline_local_includes (#276)

* Make sure to select the correct INCLUDE_PATH (#277)

* Fix typo (#278)

* [pre-commit.ci] pre-commit autoupdate (#274)

updates:
- [github.com/pre-commit/mirrors-clang-format: v18.1.4 → v18.1.5](pre-commit/mirrors-clang-format@v18.1.4...v18.1.5)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Add inlined headers as dependencies in target_embed_source (#279)

* Add inlined headers as dependencies in target_embed_source

This ensure that the embedded sources are rebuild when a header is changed

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update cmake/cudawrappers-helper.cmake

Co-authored-by: Bram Veenboer <veenboer@astron.nl>

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Bram Veenboer <veenboer@astron.nl>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add HIP version of occupancyMaxActiveBlocksPerMultiprocessor

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: John Romein <romein@astron.nl>
Co-authored-by: john-romein <39552858+john-romein@users.noreply.github.com>
Co-authored-by: Bram Veenboer <veenboer@astron.nl>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
csbnw added a commit that referenced this pull request Jul 9, 2024
@loostrum @john-romein
@csbnw @pre-commit-ci
Merge main into hipified (#282)
ab147ad 
* Added cu::Function::occupancyMaxActiveBlocksPerMultiprocessor()

* Let target_embed_source inline local header files (#263)

* Fix working directory for linking source into object file (#264)

* Update readthedocs configuration (#265)

* Fix target_embed_source when input file is a relative path (#266)

* Removed deprecated cu::Context::setSharedMemConfig (#267)

* Extend api (#268)

* Add Device::getUUID
* Add initial nvml wrapper

* Fix target_embed_source (#269)

* Fix target embed source paths (#270)

* Fix path given to ld in target_embed_source
* Fix missing arg in string replace

* Fix CI (#271)

* Add runtime subpackage
* Fix symbols for test_nvrtc

* [pre-commit.ci] pre-commit autoupdate (#259)

* Update clang-format: v17.0.6 → v18.1.4
* Replace cmakelang with clang-format and cmake-format

* Add nvrtc::findIncludePath (#272)

* Make nvrtc::findIncludePath inline (#273)

* Fix string replacements (#275)

* Fix inline_local_includes (#276)

* Make sure to select the correct INCLUDE_PATH (#277)

* Fix typo (#278)

* [pre-commit.ci] pre-commit autoupdate (#274)

updates:
- [github.com/pre-commit/mirrors-clang-format: v18.1.4 → v18.1.5](pre-commit/mirrors-clang-format@v18.1.4...v18.1.5)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>

* Add inlined headers as dependencies in target_embed_source (#279)

* Add inlined headers as dependencies in target_embed_source

This ensure that the embedded sources are rebuild when a header is changed

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Update cmake/cudawrappers-helper.cmake

Co-authored-by: Bram Veenboer <veenboer@astron.nl>

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Bram Veenboer <veenboer@astron.nl>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Add HIP version of occupancyMaxActiveBlocksPerMultiprocessor

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: John Romein <romein@astron.nl>
Co-authored-by: john-romein <39552858+john-romein@users.noreply.github.com>
Co-authored-by: Bram Veenboer <veenboer@astron.nl>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matmanc matmanc matmanc approved these changes

Assignees

@csbnw csbnw

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@csbnw @matmanc