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

Inference regression of union splitting in 1.2 vs 1.1 #32452

Closed
KristofferC opened this issue Jun 29, 2019 · 1 comment
Closed

Inference regression of union splitting in 1.2 vs 1.1 #32452

KristofferC opened this issue Jun 29, 2019 · 1 comment
Assignees
Labels
compiler:inference Type inference kind:potential benchmark Could make a good benchmark in BaseBenchmarks

Comments

@KristofferC
Copy link
Sponsor Member

KristofferC commented Jun 29, 2019

Consider the code posted in https://discourse.julialang.org/t/significant-decrease-in-performance-after-seemingly-irrelevant-changes/25796/13?u=kristoffer.carlsson, reposted here:

abstract type ABCs end

struct A <: ABCs
    x::Float64
    y::Vector{Float64}
    z::Int
end
struct B <: ABCs
    x::Float64
    y::Vector{Float64}
    z::String
end

function getSumX(letters::Tuple{Vararg{ABCs}})
    sum = 0.0
    for i = 1:length(letters)
        sum += letters[i].x
    end
    return sum
end

a1 = A(2.0, [1.0], 1)
a2 = A(3.0, [1.0], 1)
b = B(4.0, [1.0], "1")

@code_warntype getSumX((a1, a2, b))

On 1.1, the return value is correctly inferred and performance is good:

julia> @code_warntype getSumX((a1, a2, b))
Body::Float64
1 ──       goto #17 if not true
2 ┄─ %2  = φ (#1 => 0.0, #16 => %32)::Float64
│    %3  = φ (#1 => 1, #16 => %38)::Int64
│    %4  = φ (#1 => 1, #16 => %39)::Int64
│    %5  = (Base.getfield)(letters, %3, true)::Union{A, B}
│    %6  = (isa)(%5, A)::Bool
└───       goto #4 if not %6
3 ── %8  = π (%5, A)
│    %9  = (Base.getfield)(%8, :x)::Float64
└───       goto #7
4 ── %11 = (isa)(%5, B)::Bool
└───       goto #6 if not %11
5 ── %13 = π (%5, B)
│    %14 = (Base.getfield)(%13, :x)::Union{Float64, Array{Float64,1}, String}
└───       goto #7
6 ──       (Core.throw)(ErrorException("fatal error in type inference (type bound)"))
└───       $(Expr(:unreachable))
7 ┄─ %18 = φ (#3 => %9, #5 => %14)::Union{Float64, Int64, Array{Float64,1}, String}
│    %19 = (isa)(%18, Float64)::Bool
└───       goto #9 if not %19
8 ── %21 = π (%18, Float64)
│    %22 = (Base.add_float)(%2, %21)::Float64
└───       goto #12
9 ── %24 = (isa)(%18, Int64)::Bool
└───       goto #11 if not %24
10 ─ %26 = π (%18, Int64)
│    %27 = (Base.sitofp)(Float64, %26)::Float64
│    %28 = (Base.add_float)(%2, %27)::Float64
└───       goto #12
11 ─ %30 = (%2 + %18)::Float64
└───       goto #12
12 ┄ %32 = φ (#8 => %22, #10 => %28, #11 => %30)::Float64
│    %33 = (%4 === 3)::Bool
└───       goto #14 if not %33
13 ─       goto #15
14 ─ %36 = (Base.add_int)(%4, 1)::Int64
└───       goto #15
15 ┄ %38 = φ (#14 => %36)::Int64
│    %39 = φ (#14 => %36)::Int64
│    %40 = φ (#13 => true, #14 => false)::Bool
│    %41 = (Base.not_int)(%40)::Bool
└───       goto #17 if not %41
16 ─       goto #2
17 ┄ %44 = φ (#15 => %32, #1 => 0.0)::Float64
└───       return %44
julia> @btime getSumX($(a1, a2, b))
  4.578 ns (0 allocations: 0 bytes)
9.0

On the backport branch of 1.2-rc2 we instead have:

ulia> @code_warntype optimize=true getSumX((a1, a2, b))
Variables
  #self#::Core.Compiler.Const(getSumX, false)
  letters::Tuple{A,A,B}
  sum::Any
  @_4::Union{Nothing, Tuple{Int64,Int64}}
  i::Int64

Body::Any
1 ──       goto #12 if not true
2 ┄─ %2  = φ (#1 => 0.0, #11 => %19)::Any
│    %3  = φ (#1 => 1, #11 => %25)::Int64
│    %4  = φ (#1 => 1, #11 => %26)::Int64
│    %5  = Base.getfield(letters, %3, true)::Union{A, B}
│    %6  = (isa)(%5, A)::Bool
└───       goto #4 if not %6
3 ── %8  = π (%5, A)
│    %9  = Base.getfield(%8, :x)::Float64
└───       goto #7
4 ── %11 = (isa)(%5, B)::Bool
└───       goto #6 if not %11
5 ── %13 = π (%5, B)
│    %14 = Base.getfield(%13, :x)::Union{Float64, Array{Float64,1}, String}
└───       goto #7
6 ──       Core.throw(ErrorException("fatal error in type inference (type bound)"))
└───       $(Expr(:unreachable))
7 ┄─ %18 = φ (#3 => %9, #5 => %14)::Any
│    %19 = (%2 + %18)::Any
│    %20 = (%4 === 3)::Bool
└───       goto #9 if not %20
8 ──       goto #10
9 ── %23 = Base.add_int(%4, 1)::Int64
└───       goto #10
10 ┄ %25 = φ (#9 => %23)::Int64
│    %26 = φ (#9 => %23)::Int64
│    %27 = φ (#8 => true, #9 => false)::Bool
│    %28 = Base.not_int(%27)::Bool
└───       goto #12 if not %28
11 ─       goto #2
12 ┄ %31 = φ (#10 => %19, #1 => 0.0)::Any
└───       return %31

and performance is not so good:

julia> @btime getSumX($(a1, a2, b))
  78.359 ns (6 allocations: 96 bytes)
9.0

The

4 ── %11 = (isa)(%5, B)::Bool
└───       goto #6 if not %11
5 ── %13 = π (%5, B)
│    %14 = Base.getfield(%13, :x)::Union{Float64, Array{Float64,1}, String}

looks kinda odd to me. Since we for %14 know that %5 isa B why does the getfield not infer to Float64?

@KristofferC KristofferC added performance Must go faster kind:regression Regression in behavior compared to a previous version labels Jun 29, 2019
@ararslan ararslan added this to the 1.2 milestone Jun 29, 2019
@KristofferC KristofferC added the kind:potential benchmark Could make a good benchmark in BaseBenchmarks label Jul 1, 2019
@vtjnash vtjnash self-assigned this Jul 3, 2019
@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 4, 2019

Bisect confirms this is the intended behavior, changed by #30833 from Union{Float64, Int64, Array{Float64,1}, String} ==> Any.

Not related to the original issue, what I find strange about this result is just that inlining appears to be doing a bad job at marking the result expression types consistently for the user output. In the following expression, we know that %9 and %14 came from the same statement, so we'd expect them to typically have the same type, but it seems like the optimizer isn't handling those as well as would be preferred:

3 ── %8  = π (%5, A)
│    %9  = (Base.getfield)(%8, :x)::Float64
└───       goto #7
4 ── %11 = (isa)(%5, B)::Bool
└───       goto #6 if not %11
5 ── %13 = π (%5, B)
│    %14 = (Base.getfield)(%13, :x)::Union{Float64, Array{Float64,1}, String}
└───       goto #7
6 ──       (Core.throw)(ErrorException("fatal error in type inference (type bound)"))
└───       $(Expr(:unreachable))

@vtjnash vtjnash closed this as completed Jul 4, 2019
@vtjnash vtjnash added compiler:inference Type inference and removed performance Must go faster kind:regression Regression in behavior compared to a previous version labels Jul 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference kind:potential benchmark Could make a good benchmark in BaseBenchmarks
Projects
None yet
Development

No branches or pull requests

3 participants