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

Avoid unalias() for matching Array{T} when broadcasting #52286

Closed
wants to merge 1 commit into from

Conversation

topolarity
Copy link
Member

It would be good to some review of the reasoning here.

I am assuming that:

  • Base.mightalias considers two Array{T} to be aliased iff they have the same base pointer in memory
  • broadcast!(f, A, ..., A, ...) enforces that the two arrays have the same dimensions

This implies that when we do an unaliascopy() for arrays of the same type, the arrays were aligned element-for-element in memory anyway, which is exactly the case where it is safe to not make a defensive copy due to the traversal mentioned just above.

`Base.mightalias` considers two arrays to be aliased iff they have the same
base pointer in memory, which is exactly the case where it is safe to not
make a defensive copy due to the traversal mentioned above
@N5N3
Copy link
Member

N5N3 commented Nov 23, 2023

broadcast!(f, A, ..., A, ...) enforces that the two arrays have the same dimensions

This seems wrong?

Theoretically, we can unsafe_uwrap the same pointer twice with different dimension, (e.g. A with (5, 5) vs B with (5, 1))
This would change the result of A .+= B

@topolarity
Copy link
Member Author

topolarity commented Nov 23, 2023

You're right:

julia> A = collect(reshape(1.0:9.0, (3,3)))
3×3 Matrix{Float64}:
 1.0  4.0  7.0
 2.0  5.0  8.0
 3.0  6.0  9.0

julia> B = unsafe_wrap(Matrix{Float64}, pointer(A), (1,1))
1×1 Matrix{Float64}:
 1.0

With this PR this incorrectly gives:

julia> @. A = A + B
3×3 Matrix{Float64}:
2.0  6.0   9.0
4.0  7.0  10.0
5.0  8.0  11.0

It is pretty unsatisfying, since the only real source of Array{T} aliasing that I'm aware of is Base.unsafe_wrap, for which the Base.mightalias check is extremely unsound, and then reshape.

@vtjnash
Copy link
Member

vtjnash commented Nov 23, 2023

Theoretically, we can unsafe_uwrap the same pointer twice

That is unsafe anyways (it is in the name), so not defined behavior. But you could reshape the array and then pop! from one and popfirst! from the other and end up with two arrays that alias but have different offsets

@topolarity
Copy link
Member Author

topolarity commented Nov 25, 2023

But you could reshape the array and then pop! from one and popfirst! from the other and end up with two arrays that alias but have different offsets

That is true, but it's also pretty unexpected that push! and pop! alone can end up mutating an aliased array:

julia> x = ones(1,5)
1×5 Matrix{Float64}:
 1.0  1.0  1.0  1.0  1.0

julia> y = reshape(x, (5,)); pop!(y); pop!(y); pop!(y); pop!(y); push!(y, -Inf)
2-element Vector{Float64}:
   1.0
 -Inf

julia> x
1×5 Matrix{Float64}:
 1.0  -Inf  1.0  1.0  1.0

I don't think it's specified (1) whether pop!/push! preserve aliasing and (2) whether they can mutate aliases like this. Indeed, before the Memory{T} changes pop! used to copy y.

Which means that if I want to write code that does reshape -> pop! -> aliased broadcast, it is probably incorrect in Julia 1.11 if it does any push! after the pop! or if it reads from the aliased sources after the broadcast.

I think our aliasing guarantees for Array{T} are too implicit and vary too much version-to-version for you to write code that correctly (intentionally) constructs multiple aliases and then mutates them in-place.

@vtjnash
Copy link
Member

vtjnash commented Nov 26, 2023

That is somewhat true, but that why there is the unalias call here. You could also get into the same situation by calling popfirst first, then reshape (making a view), then pushfirst. After #52049, there will also be an official direct API to access this without the complications of interleaving push and reshape calls

@vtjnash
Copy link
Member

vtjnash commented Nov 26, 2023

You might want to look at #26237, #50820, and #51753, #26865. A more general version of that would also be able to check if the bounds and direction of access are compatible (e.g. what some of the unsafe_copyto! methods already do with pointer to implement an efficient memmove without needing the unaliasing copy)

@vtjnash vtjnash closed this Dec 12, 2023
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.

3 participants