-
Notifications
You must be signed in to change notification settings - Fork 435
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
Adding some comments to burn fusion #2130
Adding some comments to burn fusion #2130
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2130 +/- ##
=======================================
Coverage 86.29% 86.29%
=======================================
Files 694 694
Lines 89005 89006 +1
=======================================
+ Hits 76806 76807 +1
Misses 12199 12199 ☔ View full report in Codecov by Sentry. |
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.
Everything looks good except the comments about the relative & global IDs.
/// List of operation descriptions. These contain the exact tensor IDs | ||
/// and shapes so that kernels can be optimized correctly. | ||
/// | ||
/// The length of this list is the same as the length of the `operations` list. | ||
pub(crate) global: Vec<OperationDescription>, | ||
/// List of operation descriptions. The tensor IDs and shapes are relative | ||
/// because we don't need to know the exact values to determine which operations | ||
/// can be fused. |
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.
I think the comments around the global and relative IDs are wrong. The relative ID is needed to optimize the kernel correctly, and the global one is necessary to execute the optimization in a given execution context.
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.
So
- relative IDs so that we can fuse/optimize kernels correctly without knowing their exact shape?
- absolute IDs is to execute kernels at runtime? The number of cubes, etc. might depend on the exact shape?
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.
Yes, but also the absolute IDs are always changing; it's a stream of tensor operations, not fixed tensor IDs. So, we will reuse the same optimization based on the comparison of the relative ID, since the same streaming pattern normally arises multiple times.
This PR has been marked as stale because it has not been updated for over a month |
Pull Request Template
I've been trying to follow along the burn-fusion code; and I added some comments that helped me understand the flow.
Maybe some of them could be helpful to others as well
Checklist
run-checks all
script has been executed.