-
Notifications
You must be signed in to change notification settings - Fork 268
[CK TILE] remove dependency on std chrono #3599
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR removes the dependency on <chrono> from the CK TILE library's timer implementation to improve compilation times. The heavyweight <chrono> header is replaced with lightweight platform-specific C functions for Windows (QueryPerformanceCounter) and Linux/Unix (clock_gettime).
Changes:
- Replaced
std::chronotimer implementation with custom high-resolution clock functions - Created new
high_res_cpu_clock.hppheader with platform-specific timing implementations - Updated
cpu_timerstruct to use the new timing functions
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| include/ck_tile/host/timer.hpp | Replaced chrono-based timer with custom high_res_now() functions and timepoint_t type |
| include/ck_tile/host/high_res_cpu_clock.hpp | Added new header implementing platform-specific high-resolution timing using C APIs |
| include/ck_tile/host.hpp | Added include for the new high_res_cpu_clock.hpp header |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
edbac19 to
f0ae519
Compare
tenpercent
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since we're replacing some stdlib functionality with our custom one, should we also add tests for a sanity check?
it's still using standard timing functions, just the ones defined in <time.h> or <windows.h> headers instead of <chrono.h> . those are pretty common and tested functions, so i'm sure we can trust them. |
|
not sure why the miopen fp32 convolution failed. just restarted TheRock CI build. |
* [CK TILE] remove dependency on std chrono * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> --------- Co-authored-by: Copilot <[email protected]>
Proposed changes
I did some more digging and found that chrono is overkill for what we need. Including is notoriously slow because it is a heavyweight header. It doesn't just define a few functions; it brings in a massive web of templates, type traits, and extensive timezone and calendar support.
What we need is just a simple timer.
I replace it chrono with some C function. ( for windows and linux)
Tracking issue: #3575
Checklist
Please put an
xinto the boxes that apply. You can also fill these out after creating the PR. If you're not sure, please don't hesitate to ask.clang-formaton all changed filesDiscussion
If this is a relatively large or complex change, feel free to start a discussion by explaining why you chose the solution you did and what alternatives you considered