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/constant propagation regression in Julia 1.7 #43368

Closed
mateuszbaran opened this issue Dec 8, 2021 · 6 comments · Fixed by #50694
Closed

Inference/constant propagation regression in Julia 1.7 #43368

mateuszbaran opened this issue Dec 8, 2021 · 6 comments · Fixed by #50694
Assignees
Labels
compiler:inference Type inference kind:regression Regression in behavior compared to a previous version

Comments

@mateuszbaran
Copy link
Contributor

I've run into a problem that affects Interpolations.jl. Type inference (or more likely constant propagation?) in Julia 1.7.0 gives up earlier than in previous versions of Julia.
Fairly minimal example:

using Interpolations
nx = 10
f1(x) = sin((x-3)*2pi/(nx-1) - 1)
A = Float64[f1(x) for x in 1:nx]
itp = interpolate(A, BSpline(Constant()))
etp = extrapolate(itp, 0.0)
@code_warntype ndims(etp)

With the result:

julia> @code_warntype ndims(etp)
MethodInstance for ndims(::Interpolations.FilledExtrapolation{Float64, 1, Interpolations.BSplineInterpolation{Float64, 1, Vector{Float64}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Tuple{Base.OneTo{Int64}}}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Float64})
  from ndims(itp::AbstractInterpolation) in Interpolations at /home/mateusz/.julia/dev/Interpolations/src/Interpolations.jl:158
Arguments
  #self#::Core.Const(ndims)
  itp::Interpolations.FilledExtrapolation{Float64, 1, Interpolations.BSplineInterpolation{Float64, 1, Vector{Float64}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Tuple{Base.OneTo{Int64}}}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Float64}
Body::Any
1%1 = Interpolations.typeof(itp)::Core.Const(Interpolations.FilledExtrapolation{Float64, 1, Interpolations.BSplineInterpolation{Float64, 1, Vector{Float64}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Tuple{Base.OneTo{Int64}}}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Float64})
│   %2 = Interpolations.ndims(%1)::Any
└──      return %2

While for example using Julia 1.6.1:

julia> @code_warntype ndims(etp)
Variables
  #self#::Core.Const(ndims)
  itp::Interpolations.FilledExtrapolation{Float64, 1, Interpolations.BSplineInterpolation{Float64, 1, Vector{Float64}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Tuple{Base.OneTo{Int64}}}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Float64}

Body::Int64
1%1 = Interpolations.typeof(itp)::Core.Const(Interpolations.FilledExtrapolation{Float64, 1, Interpolations.BSplineInterpolation{Float64, 1, Vector{Float64}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Tuple{Base.OneTo{Int64}}}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Float64})
│   %2 = Interpolations.ndims(%1)::Core.Const(1)
└──      return %2

Forcing specialization for one more method of ndims solves the problem but I don't know if this is the right solution.

Versioninfo for reference:

julia> versioninfo()
Julia Version 1.7.0
Commit 3bf9d17731 (2021-11-30 12:12 UTC)
Platform Info:
  OS: Linux (x86_64-pc-linux-gnu)
  CPU: Intel(R) Core(TM) i7-4800MQ CPU @ 2.70GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-12.0.1 (ORCJIT, haswell)
Environment:
  JULIA_EDITOR = code
  JULIA_NUM_THREADS = 

ref: JuliaMath/Interpolations.jl#468

@timholy
Copy link
Sponsor Member

timholy commented Dec 8, 2021

Worth checking if #43347 fixes this too, CC @aviatesk

@dkarrasch dkarrasch added compiler:inference Type inference kind:regression Regression in behavior compared to a previous version labels Dec 8, 2021
@aviatesk
Copy link
Sponsor Member

aviatesk commented Dec 8, 2021

Optimization isn't able to derive new inter-procedural information like this (return type), so it seems to be a regression in inference. Will look into this when I have time.

@aviatesk aviatesk self-assigned this Dec 8, 2021
@N5N3
Copy link
Member

N5N3 commented Dec 9, 2021

Replacing

ndims(::Type{AbstractInterpolation{T,N,IT}}) where {T,N,IT<:DimSpec{InterpolationType}} = N
ndims(::Type{ITP}) where {ITP<:AbstractInterpolation} = ndims(supertype(ITP))

with

ndims(::Type{<:AbstractInterpolation{T,N,<:DimSpec{InterpolationType}}}) where {T,N} = N

could fix it.
The original definition leads to a recursive call of:

julia> t = typeof(etp)
Interpolations.FilledExtrapolation{Float64, 1, Interpolations.BSplineInterpolation{Float64, 1, Vector{Float64}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Tuple{Base.OneTo{Int64}}}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Float64}
julia> t = supertype(t)
AbstractExtrapolation{Float64, 1, Interpolations.BSplineInterpolation{Float64, 1, Vector{Float64}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Tuple{Base.OneTo{Int64}}}, BSpline{Constant{Nearest, Throw{OnGrid}}}}
julia> t = supertype(t)
Interpolations.AbstractInterpolationWrapper{Float64, 1, Interpolations.BSplineInterpolation{Float64, 1, Vector{Float64}, BSpline{Constant{Nearest, Throw{OnGrid}}}, Tuple{Base.OneTo{Int64}}}, BSpline{Constant{Nearest, Throw{OnGrid}}}}
julia> t = supertype(t)
AbstractInterpolation{Float64, 1, BSpline{Constant{Nearest, Throw{OnGrid}}}}

So this seems also related with the type-more-complex check?

@maleadt
Copy link
Member

maleadt commented Dec 9, 2021

Bisected to:

e8caa2750969b1f4e8e41431ca8d1840aa8ac51b is the first bad commit
commit e8caa2750969b1f4e8e41431ca8d1840aa8ac51b
Author: Jameson Nash <vtjnash@gmail.com>
Date:   Mon Nov 1 15:10:32 2021 -0400

    inference: relax type_more_complex for Type
    
    If the object is not a potential Type{...}, we can allow greater
    flexibility in recursion.
    
    (cherry picked from commit 1daaf473d1c268ce8b54f47bd7b30c2f74d66b99)

Same as #43296. cc @vtjnash

@maleadt
Copy link
Member

maleadt commented Dec 9, 2021

MWE:

abstract type e{a,j} <: AbstractArray{a,j} end
abstract type b{a,j,c,d} <: e{a,j} end
struct h{a,j,f,d,i} <: b{a,j,f,d} end
Base.ndims(::Type{f}) where f<:e = ndims(supertype(f))
Base.ndims(g::e) = ndims(typeof(g))
Base.return_types(ndims, (h{Any, 0, Any, Int64, Any},))

Int64 on 1.6, Any on 1.7

@vtjnash
Copy link
Sponsor Member

vtjnash commented Jul 27, 2023

Note that OP is fixed, due to changes to ndims, but the MWE above still works.

vtjnash added a commit that referenced this issue Jul 27, 2023
We had a special case for Type that disallowed type trait recursion in
favor of a pattern that almost never appears in code (only once in the
compiler by accident where it doesn't matter). This was unnecessarily
confusing and unexpected to predict what can infer, and made traits
harder than necessary (such as Broadcast.ndims since 70fc3cd).

Fix #43296
Fix #43368
vtjnash added a commit that referenced this issue Jul 31, 2023
We had a special case for Type that disallowed type trait recursion in
favor of a pattern that almost never appears in code (only once in the
compiler by accident where it doesn't matter). This was unnecessarily
confusing and unexpected to predict what can infer, and made traits
harder than necessary (such as Broadcast.ndims since 70fc3cd).

Fix #43296
Fix #43368
vtjnash added a commit that referenced this issue Aug 2, 2023
We had a special case for Type that disallowed type trait recursion in
favor of a pattern that almost never appears in code (only once in the
compiler by accident where it doesn't matter). This was unnecessarily
confusing and unexpected to predict what can infer, and made traits
harder than necessary (such as Broadcast.ndims since 70fc3cd).

Fix #43296
Fix #43368
IanButterworth pushed a commit that referenced this issue Aug 19, 2023
We had a special case for Type that disallowed type trait recursion in
favor of a pattern that almost never appears in code (only once in the
compiler by accident where it doesn't matter). This was unnecessarily
confusing and unexpected to predict what can infer, and made traits
harder than necessary (such as Broadcast.ndims since 70fc3cd).

Fix #43296
Fix #43368

(cherry picked from commit 33e3d9f)
nalimilan pushed a commit that referenced this issue Nov 5, 2023
We had a special case for Type that disallowed type trait recursion in
favor of a pattern that almost never appears in code (only once in the
compiler by accident where it doesn't matter). This was unnecessarily
confusing and unexpected to predict what can infer, and made traits
harder than necessary (such as Broadcast.ndims since 70fc3cd).

Fix #43296
Fix #43368

(cherry picked from commit 33e3d9f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference kind:regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants