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

ENH: Implement broadcast_to function #782

Merged
merged 2 commits into from
Oct 2, 2024
Merged

Conversation

mtsokol
Copy link
Collaborator

@mtsokol mtsokol commented Sep 30, 2024

Hi @hameerabbasi,

This PR adds broadcast_to Array API function.

It it only broadcasts missing dimensions, for 1-size dimension it can't broadcast as it's a limitation of linalg.broadcast in MLIR.

@mtsokol mtsokol self-assigned this Sep 30, 2024
Copy link

codspeed-hq bot commented Sep 30, 2024

CodSpeed Performance Report

Merging #782 will degrade performances by 12.39%

Comparing broadcast_to-func (8448d56) with main (1593a0f)

Summary

⚡ 10 improvements
❌ 1 regressions
✅ 329 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark main broadcast_to-func Change
test_index_fancy[side=100-rank=1-format='coo'] 1,417 µs 639.6 µs ×2.2
test_index_slice[side=100-rank=2-format='gcxs'] 2.4 ms 2.7 ms -12.39%
test_elemwise[f=<built-in function add>-backend='Finch'-side=1000] 1.9 ms 1.5 ms +28.29%
test_elemwise[f=<built-in function add>-backend='Finch'-side=100] 1,148.7 µs 714.3 µs +60.81%
test_elemwise[f=<built-in function add>-backend='Finch'-side=500] 1,298.4 µs 881.9 µs +47.23%
test_elemwise[f=<built-in function gt>-backend='Finch'-side=1000] 1.7 ms 1.4 ms +21.24%
test_elemwise[f=<built-in function gt>-backend='Finch'-side=100] 982.4 µs 705.8 µs +39.2%
test_elemwise[f=<built-in function gt>-backend='Finch'-side=500] 1,187.4 µs 877.5 µs +35.31%
test_elemwise[f=<built-in function mul>-backend='Finch'-side=1000] 1,149.7 µs 735.3 µs +56.35%
test_elemwise[f=<built-in function mul>-backend='Finch'-side=100] 1,110.3 µs 691.2 µs +60.64%
test_elemwise[f=<built-in function mul>-backend='Finch'-side=500] 1,127 µs 712.2 µs +58.24%

@hameerabbasi
Copy link
Collaborator

I'm hesitant about merging this as it contains lots of heuristics to figure out the output format -- not to mention there should be a "broadcast level" that's the equivalent of a "zero stride", or perhaps we can ignore the broadcast indices on the Python side.

@mtsokol mtsokol force-pushed the broadcast_to-func branch from e46cd64 to ca55bf7 Compare October 2, 2024 10:39
@mtsokol
Copy link
Collaborator Author

mtsokol commented Oct 2, 2024

I'm hesitant about merging this as it contains lots of heuristics to figure out the output format -- not to mention there should be a "broadcast level" that's the equivalent of a "zero stride", or perhaps we can ignore the broadcast indices on the Python side.

The heuristics are gone (they're already added by a different PR). What do you mean by a "broadcast level"?

@mtsokol mtsokol added the enhancement Indicates new feature requests label Oct 2, 2024
@hameerabbasi
Copy link
Collaborator

What do you mean by a "broadcast level"?

Something that represents a zero-stride, or an ignored index.

@mtsokol
Copy link
Collaborator Author

mtsokol commented Oct 2, 2024

Can you give an Array API broadcast_to(...) example call that uses ignored index? I'm not sure which feature you mean.

@mtsokol
Copy link
Collaborator Author

mtsokol commented Oct 2, 2024

Decided that for now broadcast_to is a copying operation and Tensor views should be included on the MLIR or Finch Dialect side.

@mtsokol mtsokol mentioned this pull request Oct 2, 2024
Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

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

Thanks, @mtsokol!

@mtsokol mtsokol enabled auto-merge October 2, 2024 13:03
@hameerabbasi hameerabbasi disabled auto-merge October 2, 2024 13:46
@hameerabbasi hameerabbasi merged commit db60537 into main Oct 2, 2024
15 of 16 checks passed
@hameerabbasi hameerabbasi deleted the broadcast_to-func branch October 2, 2024 13:46
@hameerabbasi
Copy link
Collaborator

Force-merged due to the CI hiccup. Thanks, @mtsokol!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants