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

RFC: Fix #31392, unaliasing of broadcast arguments against destinations with repeated indices #31407

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 26 additions & 7 deletions base/broadcast.jl
Original file line number Diff line number Diff line change
Expand Up @@ -969,15 +969,34 @@ end
end
end

# For broadcasted assignments like `broadcast!(f, A, ..., A, ...)`, where `A`
# appears on both the LHS and the RHS of the `.=`, then we know we're only
# going to make one pass through the array, and even though `A` is aliasing
# against itself, the mutations won't affect the result as the indices on the
# LHS and RHS will always match. This is not true in general, but with the `.op=`
# syntax it's fairly common for an argument to be `===` a source.
broadcast_unalias(dest, src) = dest === src ? src : unalias(dest, src)
"""
broadcast_unalias(dest, src)

`broadcast_unalias` is a simple extension of `Base.unalias` to enable an important
performance optimization specific to the semantics of broadcasting.

For broadcasted assignments like `broadcast!(f, A, ..., A, ...)`, where `A`
appears on both the LHS and the RHS of the `.=`, then we know we're only
going to make one pass through the array, and even though `A` is aliasing
against itself, the mutations won't affect the result as the indices on the
LHS and RHS will always match. As long as the indices do not repeat, the
RHS does not need to be unaliased. This is particuclarly valuable with the `.op=`
syntax since that makes it fairly common for an argument to be `===` a source.
"""
broadcast_unalias(dest, src) = dest === src && !potentially_self_aliased(dest) ? src : unalias(dest, src)
broadcast_unalias(::Nothing, src) = src

"""
potentially_self_aliased(A)
Copy link
Member Author

@mbauman mbauman May 13, 2021

Choose a reason for hiding this comment

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

A year Two years later, I'm not a big fan of this name. I'm guessing I used potentially as an attempt to say that trues are conclusive but falses are not... but I don't think it actually reads that way.

mightalias documents itself as a "conservative" test for sharing the same memory, but I don't even know what that means on its face. Is it more likely to have false positive or a false negative?

In both cases, I used a different word than my usual go-to "maybe_foo" as an attempt to do some work in one direction or the other, but I don't think it was successful because I'm having a hard time piecing together what they mean right now. In short, here's what we have between these two systems:

function returns wrong if...
mightalias true the arrays reference distinct subsets of the same memory region in a complicated manner
mightalias false a custom array has not tracked its internal field(s) with dataids and one of those aliases
potentially_self_aliased true an array goofed its implementation (the default definitions should not have false positives)
potentially_self_aliased false this is the default answer; a self-aliasing array didn't define a method on this obscure function

Copy link
Member Author

Choose a reason for hiding this comment

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

Numpy calls this may_have_internal_overlap with tri-state output (known to not overlap, known to overlap, and unknown).


Returns true if multiple locations in `A` reference the same memory
"""
potentially_self_aliased(::DenseArray) = false
potentially_self_aliased(A::StridedArray) = any(==(0), strides(A))
Copy link
Member Author

Choose a reason for hiding this comment

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

potentially_self_aliased(A::SubArray) = any(map(!allunique, A.indices))
Copy link
Member Author

Choose a reason for hiding this comment

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

Just a note for posterity: any(map(!allunique, A.indices)) does better compile-time evaluation with heterogeneous tuples and constant-folds to return 0 more frequently.

potentially_self_aliased(A::Union{Base.ReshapedArray,Base.ReinterpretArray}) = potentially_self_aliased(A.parent)
potentially_self_aliased(::Any) = false

# Preprocessing a `Broadcasted` does two things:
# * unaliases any arguments from `dest`
# * "extrudes" the arguments where it is advantageous to pre-compute the broadcasted indices
Expand Down
2 changes: 1 addition & 1 deletion base/set.jl
Original file line number Diff line number Diff line change
Expand Up @@ -416,8 +416,8 @@ function allunique(C)
end

allunique(::Union{AbstractSet,AbstractDict}) = true

allunique(r::AbstractRange) = !iszero(step(r)) || length(r) <= 1
allunique(::Number) = true

filter!(f, s::Set) = unsafe_filter!(f, s)

Expand Down
33 changes: 33 additions & 0 deletions test/broadcast.jl
Original file line number Diff line number Diff line change
Expand Up @@ -800,6 +800,39 @@ end
@test iszero.((big(0):big(2)) .- 1) == [false, true, false]
end

@testset "Issue #31392: In-place broadcasting with repeated indices" begin
x = [3]
@views x[[1,1]] .= x[[1,1]] .+ x[[1,1]]
@test x[] == 6
x[[1,1]] .+= 1
@test x[] == 7

x = [3]
@views x[[1,1]] .+= x[[1,1]]
@test x[] == 6
x[[1,1]] .+= 1
@test x[] == 7

for I in ([1,1], StepRangeLen(1, 0, 2))
x = [3]
@views x[I] .= x[I] .+ x[I]
@test x[] == 6

x = [3]
@views x[I] .+= x[I]
@test x[] == 6

for v in (view([3], I), reshape(view([3], I), 1, 2))
v .= v .+ v
@test v[1] == 6
v .+= v
@test v[1] == 12
v .+= 1
@test v[1] == 13
end
end
end

@testset "Issue #27775: Broadcast!ing over nested scalar operations" begin
a = zeros(2)
a .= 1 ./ (1 + 2)
Expand Down