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

Add RMM_USE_NVTX cmake option to provide localized control of NVTX for RMM #1602

Merged
merged 5 commits into from
Jul 19, 2024

Conversation

jlowe
Copy link
Member

@jlowe jlowe commented Jul 3, 2024

Description

Adds a new RMM_USE_NVTX cmake option that defaults to the prior USE_NVTX setting if not set. If RMM_USE_NVTX is OFF, it stubs out the RMM_FUNC_RANGE macro which is used for RMM's NVTX ranges, effectively disabling NVTX within RMM yet leaving it enabled for the rest of the project using RMM.

Closes #1595

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@jlowe jlowe requested review from a team as code owners July 3, 2024 21:49
@jlowe jlowe requested review from harrism and jrhemstad July 3, 2024 21:49
@github-actions github-actions bot added CMake cpp Pertains to C++ code labels Jul 3, 2024
@jlowe
Copy link
Member Author

jlowe commented Jul 3, 2024

Verified that with this change by default RMM NVTX ranges appear in profiles, but I can add -DRMM_USE_NVTX=OFF to our libcudf build to remove RMM NVTX ranges while still preserving the other NVTX ranges in libcudf.

CMakeLists.txt Outdated Show resolved Hide resolved
@jlowe jlowe requested a review from bdice July 10, 2024 15:22
@bdice
Copy link
Contributor

bdice commented Jul 11, 2024

One final question before we merge: should this be RMM_NVTX instead of RMM_USE_NVTX? We have precedent for {LIB}_NVTX (without "USE") in RAFT here: https://github.com/rapidsai/raft/blob/5bf6642b9ec4c82e97db21f1ce3bb057fc20db82/cpp/CMakeLists.txt#L68

@jlowe
Copy link
Member Author

jlowe commented Jul 11, 2024

Oh I didn't realize there was already precedence for a project-specific NVTX config. Makes sense to be consistent.

@GregoryKimball
Copy link

@robertmaynard is this ready to merge?

@jlowe
Copy link
Member Author

jlowe commented Jul 19, 2024

Can someone with permissions update the PR labels or grant me the permissions to do so?

I think this is ready to go. Would really like to see this get into the 24.08 release.

@vyasr vyasr added non-breaking Non-breaking change improvement Improvement / enhancement to an existing function labels Jul 19, 2024
@vyasr
Copy link
Contributor

vyasr commented Jul 19, 2024

/merge

@rapids-bot rapids-bot bot merged commit 5f786ba into rapidsai:branch-24.08 Jul 19, 2024
61 checks passed
@jlowe jlowe deleted the rmm-nvtx branch July 22, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake cpp Pertains to C++ code improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEA] Ability to disable RMM NVTX without disabling all NVTX
6 participants