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

Intersection of CartesianIndices #33055

Closed
rapus95 opened this issue Aug 24, 2019 · 3 comments · Fixed by #36643
Closed

Intersection of CartesianIndices #33055

rapus95 opened this issue Aug 24, 2019 · 3 comments · Fixed by #36643

Comments

@rapus95
Copy link
Contributor

rapus95 commented Aug 24, 2019

Hey,
I recently stumbled across the fact, that intersection of CartesianIndices can be quite slow and scales with the size of the ranges that are used. As such I found out that there is no specialized version for the intersection of multiple CartesianIndices. Does this have any reason? If not, if you want and if you tell me where that line

Base.intersect(a::CartesianIndices, b::CartesianIndices) = CartesianIndices(intersect.(a.indices, b.indices))

would go and what the proper naming conventions here are, I could do a pull request.
EDIT: How to handle intersect(IdentityUnitRange, IdentityUnitRange) and mixed intersect?

https://github.com/JuliaLang/julia/blob/master/base/multidimensional.jl#L385
And while looking into this file, why do you not use a short-circuiting && in this line?

@mbauman
Copy link
Member

mbauman commented Aug 26, 2019

Sure, that seems like a reasonable specialization to have. I'd put it in the IteratorsMD module inside base/multidimensional.jl. Putting it somewhere around that in function seems like a good place.

It looks like we already handle IdentityUnitRanges appropriately — this seems like the right thing to do:

julia> intersect(Base.OneTo(4), Base.OneTo(5))
Base.OneTo(4)

julia> intersect(Base.OneTo(4), 1:5)
1:4

As far as why in isn't using short-circuiting, there are cases where avoiding the branch introduced by && can be beneficial. In some cases LLVM will automatically convert && to & by itself when it thinks it's advantageous, but it has to be able to prove that the second expression doesn't have a side-effect. It's most evident inside SIMD-able loops, where failing to perform this optimization breaks SIMDification and leads to a huge (2-8x) performance loss.

@mdav2
Copy link

mdav2 commented Jul 4, 2020

What needs to be done to get this merged? The implementation of intersect above cut down my runtime by orders of magnitude, so it seems like a good idea!

@timholy
Copy link
Member

timholy commented Jul 4, 2020

Submit a pull request with some tests.

stev47 added a commit to stev47/julia that referenced this issue Jul 13, 2020
stev47 added a commit to stev47/julia that referenced this issue Jul 14, 2020
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 a pull request may close this issue.

4 participants