Skip to content

fix #9770 (type inference error) #9926

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

Closed
wants to merge 1 commit into from
Closed

fix #9770 (type inference error) #9926

wants to merge 1 commit into from

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Jan 25, 2015

here's the segfault-enhanced test case for this:

x9770 = false
function f9770(x)
    if x9770
        g9770(:a, :foo)
    else
        x
    end
end
function g9770(x,y)
   if isa(y,Symbol)
       f9770(x)
   else
       g9770(:a, :foo)
   end
end
@test g9770(:a, "c") === :a
@test g9770(:b, :c) === :b

@JeffBezanson i don't like adding all of the typeinf function to the "don't-infer" list, but it seemed to suffice to get through type inference

@vtjnash
Copy link
Member Author

vtjnash commented Jan 25, 2015

(also, with this change, we never cache g9770(::Symbol,::Symbol), which also seems incorrect)

edit: the corrected patch needs:

diff --git a/base/inference.jl b/base/inference.jl
index 3e4f091..c8c0e2d 100644
--- a/base/inference.jl
+++ b/base/inference.jl
@@ -1311,7 +1311,7 @@ function typeinf(linfo::LambdaStaticData,atypes::Tuple,sparams::Tuple, def, cop)
             # return best guess so far
             f = f::CallStack
             f.recurred = true
-            f.toprec = !f.rec
+            f.toprec = f.toprec || !f.rec
             f.rec = true
             r = inference_stack
             while !is(r, f)

but I still think that this algorithm may not be correct for handling recurred functions and may record the wrong return value. it's hard to create a failing case however.

@vtjnash
Copy link
Member Author

vtjnash commented Jan 26, 2015

here's my newest attempt at breaking it:

julia> begin
                              @noinline _true() = true
                              @noinline _false() = false
                              function f(x)
                                 if _false()
                                   g(:a, :foo)
                                 else
                                   x
                                 end
                              end
                              function g(x,y)
                                if isa(y,Symbol)
                                  f(x)
                                elseif _true()
                                  h(g(x,y))
                                end
                              end
                              h(x::Symbol) = Int
                              h(x::ASCIIString) = Float32
                              h(x::Any) = Function

       end
h (generic function with 3 methods)

julia> @code_typed g(:a, "c")
1-element Array{Any,1}:
 :($(Expr(:lambda, Any[:x,:y], Any[Any[],Any[Any[:x,Symbol,0],Any[:y,ASCIIString,0]],Any[]], :(begin  # none, line 12:
        unless $(Expr(:call1, :isa, :(y::ASCIIString), :Symbol)) goto 0 # line 13:
        return $(Expr(:call1, :f, :(x::Symbol)))
        goto 2
        0:  # line 14:
        unless $(Expr(:call1, :_true)) goto 1 # line 15:
        return h($(Expr(:call1, :g, :(x::Symbol), :(y::ASCIIString))))::Union(Type{Int64},Type{Float32},Type{Function})
        goto 2
        1: 
        return
        2: 
    end::Any))))

julia> @code_typed g(:a, :c)
1-element Array{Any,1}:
 :($(Expr(:lambda, Any[:x,:y], Any[Any[],Any[Any[:x,Symbol,0],Any[:y,Symbol,0]],Any[]], :(begin  # none, line 12:
        unless isa(y::Symbol,Symbol)::Bool goto 0 # line 13:
        return f(x::Symbol)::Any
        goto 2
        0:  # line 14:
        unless _true()::Bool goto 1 # line 15:
        return h(g(x::Symbol,y::Symbol)::Any)::Union(Type{Int64},Type{Float32},Type{Function})
        goto 2
        1: 
        return
        2: 
    end::Any))))

julia> @code_typed g(:a, "c")
1-element Array{Any,1}:
 :($(Expr(:lambda, Any[:x,:y], Any[Any[],Any[Any[:x,Symbol,0],Any[:y,ASCIIString,0]],Any[]], :(begin  # none, line 12:
        unless isa(y::ASCIIString,Symbol)::Bool goto 0 # line 13:
        return f(x::Symbol)::Any
        goto 2
        0:  # line 14:
        unless _true()::Bool goto 1 # line 15:
        return h(g(x::Symbol,y::ASCIIString)::Any)::Union(Type{Int64},Type{Float32},Type{Function})
        goto 2
        1: 
        return
        2: 
    end::Any))))

julia> @code_typed f(:a)
1-element Array{Any,1}:
 :($(Expr(:lambda, Any[:x], Any[Any[],Any[Any[:x,Symbol,0]],Any[]], :(begin  # none, line 5:
        unless _false()::Bool goto 0 # line 6:
        return g(:a,:foo)::Any
        goto 1
        0:  # line 8:
        return x::Symbol
        1: 
    end::Any))))

@tkelman
Copy link
Contributor

tkelman commented Jan 26, 2015

Is this causing appveyor to time out with a higher-than-usual probability? OSX Travis too?

@vtjnash
Copy link
Member Author

vtjnash commented Jan 27, 2015

yes, even with the above patch now included, it significantly increases build times

@JeffBezanson
Copy link
Member

Rebased. But yes, for me this took the sysimg build time (starting with inference0) from 3 minutes to 4 minutes.

@vtjnash
Copy link
Member Author

vtjnash commented Aug 21, 2015

that's actually not a bad as I feared. initially i think this was at least doubling the build time (appveyor went from 20minutes to > 40 minute). is it worthwhile to switch inference to a queue instead of recursion to do the much faster fix for this while we wait for the green-fairy (which i've been told already handles this case)?

@vtjnash vtjnash changed the title fix #9770 fix #9770 (type inference error) Nov 16, 2015
@vtjnash
Copy link
Member Author

vtjnash commented Jan 24, 2016

also fixes #9222

@tkelman
Copy link
Contributor

tkelman commented Jan 24, 2016

test? and more descriptive commit message?

@vtjnash vtjnash closed this Feb 27, 2016
@tkelman tkelman deleted the jn/9770 branch May 3, 2016 07:09
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