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

Constant propagation through broadcast #43333

Open
mikmoore opened this issue Dec 4, 2021 · 1 comment
Open

Constant propagation through broadcast #43333

mikmoore opened this issue Dec 4, 2021 · 1 comment
Labels
broadcast Applying a function over a collection

Comments

@mikmoore
Copy link
Contributor

mikmoore commented Dec 4, 2021

It would be nice if constant propagation worked through broadcasting.

using BenchmarkTools
get2(x) = getindex(x,2)
foo1(x) = sum(get2.(x))
foo2(x) = sum(getindex.(x,2))
x = fill((1,2.0,3//1),1000) # eltype(eltype(x)) is not concrete
@code_warntype foo1(x) # succeeds inference
@code_warntype foo2(x) # fails inference
@btime foo1($x); # 762.069 ns (1 allocation: 7.94 KiB)
@btime foo2($x); # 26.800 μs (2000 allocations: 70.39 KiB)

This fails because getindex(::Tuple{A,B,C},i::Int) where {A,B,C} is unstable unless A==B==C or i gets constant-folded.

One can work around this via the get2 solution above or a similar broadcast(...) do block (and obviously sum(get2,x) is the best solution in this MWE), but having broadcast natively recognize and propagate scalar constants would be convenient and prevent people from unknowingly falling into this trap. The primary cases where I've gotten into trouble with this are broadcasts over getindex and getproperty.

@N5N3
Copy link
Member

N5N3 commented Dec 5, 2021

Our broadcast support const-propagation. If you look-into @code_typed.
You will find:

11%38 = Base.Broadcast.ifelse(%10, %21, 1)::Int64%39 = Base.arrayref(false, x, %38)::Tuple{Int64, Float64, Rational{Int64}}%40 = Base.getfield(%39, 2, true)::Float64

The problem is that during broadcast, we call copy, which checks the return type of bc::Broadcasted first, if it's concrete then falls-back to copyto!.
IIUC, the current combine_eltypes is pure-type based, and of-cource it fails to return Float64.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection
Projects
None yet
Development

No branches or pull requests

3 participants