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

Add CK_TIME_KERNEL as toggleable CMake Variable #1794

Merged
merged 3 commits into from
Jan 20, 2025
Merged

Add CK_TIME_KERNEL as toggleable CMake Variable #1794

merged 3 commits into from
Jan 20, 2025

Conversation

lucbruni-amd
Copy link
Contributor

@lucbruni-amd lucbruni-amd commented Jan 6, 2025 •
edited
Loading

Allow CK_TIME_KERNEL to be toggled through CMake variable during build, maintaining default enablement

Example:
cmake <...> -D CK_TIME_KERNEL=OFF ..

@lucbruni-amd lucbruni-amd linked an issue Jan 6, 2025 that may be closed by this pull request
[Issue]: CK_TIME_KERNEL used by default #1780
Closed
@illsilin
Copy link
Collaborator

illsilin commented Jan 8, 2025

I don't think this is a good idea.
This will turn off the kernel timing in ckProfiler which is built for 2 main purposes: verifying the kernel correctness and measuring performance.
If we merged this PR today, all our performance benchmarking will get disabled.
If someone is experiencing false compiler warnings on an unsupported OS with a 1.5 year old rocm version, I suggest they address that specific issue instead of disabling one of the most important diagnostic tools in our library.

poyenc
poyenc reviewed Jan 10, 2025
View reviewed changes
CMakeLists.txt Outdated
@@ -102,6 +102,11 @@ if(CK_USE_CODEGEN)
add_definitions(-DCK_USE_CODEGEN)
endif()

option(CK_TIME_KERNEL "Enable kernel time tracking" OFF)
if(CK_TIME_KERNEL)
add_definitions(-DCK_TIME_KERNEL)
Copy link
Contributor

@poyenc poyenc Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Uers are not able to define CK_TIME_KERNEL=1 through setting the CMake option.

Copy link
Contributor Author

@lucbruni-amd lucbruni-amd Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Do you mean "not intended to be allowed", or "not able to with my changes"? I was able to compile and modify the directive with this change. If the user should not be able to modify this, please clarify. I have added a new commit to persist the default as ON while still allowing users to disable it if they please.

@lucbruni-amd lucbruni-amd reopened this Jan 10, 2025
@lucbruni-amd lucbruni-amd changed the title Disable CK_TIME_KERNEL by Default, Add as CMake Variable Add CK_TIME_KERNEL as toggleable CMake Variable Jan 10, 2025
@illsilin illsilin merged commit 3fb2f5a into develop Jan 20, 2025
10 checks passed
@lucbruni-amd lucbruni-amd deleted the lucbruni/issue-1780 branch January 20, 2025 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poyenc poyenc poyenc left review comments

@illsilin illsilin illsilin approved these changes

@junliume junliume Awaiting requested review from junliume

@carlushuang carlushuang Awaiting requested review from carlushuang carlushuang is a code owner

@qianfengz qianfengz Awaiting requested review from qianfengz qianfengz is a code owner

@aosewski aosewski Awaiting requested review from aosewski aosewski is a code owner

@geyyer geyyer Awaiting requested review from geyyer geyyer is a code owner

@bartekxk bartekxk Awaiting requested review from bartekxk bartekxk is a code owner

@andriy-ca andriy-ca Awaiting requested review from andriy-ca andriy-ca is a code owner

@afagaj afagaj Awaiting requested review from afagaj afagaj is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Issue]: CK_TIME_KERNEL used by default
3 participants
@lucbruni-amd @illsilin @poyenc