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

Custom fusion #2486

Merged
merged 15 commits into from
Nov 13, 2024
Merged

Custom fusion #2486

merged 15 commits into from
Nov 13, 2024

Conversation

ArthurBrussee
Copy link
Contributor

Pull Request Template

Related Issues/PRs

#1970

Changes

Expose enough information to allow custom operations to use fusing. Also expose a function to resolve a tensor from a fusion backend.

Draft until tracel-ai/cubecl#256 lands & some other bits are cleaned up.

This also fixes #1970 by bailing on fusing when going over max bindings.

crates/burn-fusion/src/client/base.rs Outdated Show resolved Hide resolved
Comment on lines 41 to 47
let mut cloned = self.builder.clone();
build(&mut cloned);
if cloned.estimate_bindings() > self.max_bindings {
return false;
}
self.builder = cloned;
true
Copy link
Member

Choose a reason for hiding this comment

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

cloning every time is a bit wasteful. I would only do it if builder_current_state.estimate_bindings() + new_op.nodes() > self.max_bindings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That doesn't really work - we don't know how many nodes someone is adding in the build function. And we can't easily keep track of it as adding them mutates the builder.

estimate_bindings() also seems more expensive than a clone in the first place?

I've at least added a fast path for the first operation which should be common. If that's not enough might need to rethink how these operations are added so we can count them more easily.

crates/burn-jit/src/fusion/on_write/builder.rs Outdated Show resolved Hide resolved
crates/burn-tensor/src/repr/operation.rs Outdated Show resolved Hide resolved
crates/burn-router/src/runner.rs Show resolved Hide resolved
@ArthurBrussee
Copy link
Contributor Author

I've also added a fix for a deadlock in fusion on wasm in this PR now. It might or might not make things run a bit smoothly on other platforms too, as it was incorrectly holding the server lock while waiting for a read.

@ArthurBrussee ArthurBrussee marked this pull request as ready for review November 13, 2024 20:42
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

Attention: Patch coverage is 56.29921% with 111 lines in your changes missing coverage. Please review.

Project coverage is 82.91%. Comparing base (6e71aaf) to head (fc63b22).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
crates/burn-tensor/src/repr/operation.rs 0.00% 28 Missing ⚠️
crates/burn-fusion/src/client/mutex.rs 35.13% 24 Missing ⚠️
crates/burn-fusion/src/stream/context.rs 0.00% 19 Missing ⚠️
crates/burn-fusion/src/server.rs 40.00% 18 Missing ⚠️
...s/burn-jit/src/kernel/conv/conv2d/implicit_gemm.rs 0.00% 9 Missing ⚠️
...urn-jit/src/kernel/conv/deform_conv_transpose2d.rs 0.00% 2 Missing ⚠️
...rates/burn-jit/src/kernel/reduce/subcube/kernel.rs 33.33% 2 Missing ⚠️
...tes/burn-jit/src/kernel/reduce/subcube/mean_dim.rs 0.00% 2 Missing ⚠️
...tes/burn-jit/src/kernel/reduce/subcube/prod_dim.rs 0.00% 2 Missing ⚠️
...ates/burn-jit/src/kernel/reduce/subcube/sum_dim.rs 0.00% 2 Missing ⚠️
... and 3 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2486      +/-   ##
==========================================
- Coverage   82.91%   82.91%   -0.01%     
==========================================
  Files         810      812       +2     
  Lines      104904   105334     +430     
==========================================
+ Hits        86984    87336     +352     
- Misses      17920    17998      +78     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@nathanielsimard nathanielsimard merged commit c7233bf into tracel-ai:main Nov 13, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Too many bindings of type StorageBuffers in Stage ShaderStages(COMPUTE)
2 participants