Skip to content

Drop combine_size #1008

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

Merged
merged 7 commits into from
Mar 16, 2022
Merged

Drop combine_size #1008

merged 7 commits into from
Mar 16, 2022

Conversation

N5N3
Copy link
Contributor

@N5N3 N5N3 commented Mar 1, 2022

This PR tries to perform combine_size during Broadcast.instantiate:

  1. For broadcast, combine_axes will return a Tuple{Vararg{SOneTo}}, thus the output size has been static.
  2. For broadcast!, the dest's axes is marked as static (if possible). If there's no size-1 dim in src. The current generated kernal will be called.

With this change, we can reduce the compile time for first broadcast by about 1/4:

julia> a = @SArray zeros(3,3)
3×3 SMatrix{3, 3, Float64, 9} with indices SOneTo(3)×SOneTo(3):
 0.0  0.0  0.0
 0.0  0.0  0.0
 0.0  0.0  0.0

julia> @time a .+ a;
  0.234903 seconds (452.90 k allocations: 24.654 MiB, 99.75% compilation time) #  on master: 0.308612 seconds (513.89 k allocations: 27.912 MiB, 99.87% compilation time)

@N5N3 N5N3 force-pushed the Drop_combine_size branch 4 times, most recently from d55479d to 4fab441 Compare March 2, 2022 13:30
Copy link
Collaborator

@thchr thchr left a comment

Choose a reason for hiding this comment

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

Someone knowledgeable about the broadcasting machinery should review this probably - but I was reading this and figured I might as well add the few comments I had.

N5N3 and others added 5 commits March 14, 2022 22:41
add `static_combine_axes` to handle Tuple correctly
1. `axes` should be static.
2. The implementation for broadcast(1) are unified)
3. simplify `broadcast_index`
Co-Authored-By: Thomas Christensen <tchr@mit.edu>
@N5N3 N5N3 force-pushed the Drop_combine_size branch from 4fab441 to 8af7058 Compare March 14, 2022 14:41
Copy link
Collaborator

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

I've done some work with broadcasting before but I'm not quite familiar with certain parts of broadcasting touched in this PR.

@N5N3 N5N3 force-pushed the Drop_combine_size branch from 5ef24cb to 7fedea6 Compare March 15, 2022 10:28
Copy link
Collaborator

@mateuszbaran mateuszbaran left a comment

Choose a reason for hiding this comment

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

I think this looks fine 👍 .

@mateuszbaran
Copy link
Collaborator

Could you bump patch version? I'd tag a new release.

@N5N3
Copy link
Contributor Author

N5N3 commented Mar 16, 2022

Could you bump patch version? I'd tag a new release.

Patch version was bumped in #1001. (master is 1.4.2 at present). I guess there's no need to bump again?

@mateuszbaran
Copy link
Collaborator

Yes, right, I didn't notice.

@mateuszbaran mateuszbaran merged commit b09880b into JuliaArrays:master Mar 16, 2022
@N5N3 N5N3 deleted the Drop_combine_size branch March 16, 2022 09:40
@judober
Copy link
Contributor

judober commented Mar 18, 2022

I think this affects HybridArrays. See:
JuliaArrays/HybridArrays.jl#50

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.

4 participants