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

bad inferrability is back on recent Julia master #45

Closed
jrevels opened this issue May 31, 2018 · 5 comments
Closed

bad inferrability is back on recent Julia master #45

jrevels opened this issue May 31, 2018 · 5 comments

Comments

@jrevels
Copy link
Collaborator

jrevels commented May 31, 2018

julia> versioninfo()
Julia Version 0.7.0-beta.206
Commit b6f9244d6b (2018-07-08 19:42 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin17.5.0)
  CPU: Intel(R) Core(TM) i7-7920HQ CPU @ 3.10GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.0 (ORCJIT, skylake)

julia> using Cassette

julia> Cassette.@context Ctx

julia> @code_typed Cassette.recurse(Ctx(), unsafe_load, pointer(Float32[]))
CodeInfo(
105 1 ──       Core.getfield(%%##recurse_arguments#377, 1)                         │
    │    %2  = Core.getfield(%%##recurse_arguments#377, 2)::Ptr{Float32}           │
    └───       goto 2 if not false                                                 │╻         overdub
    2 ┄─       goto 3 if not false                                                 ││╻╷        unsafe_load
    3 ┄─       goto 4 if not false                                                 │││╻╷        overdub
    4 ┄─       goto 5                                                              ││││┃│        Type
    5 ── %7  = :(Core.typeassert)::typeof(typeassert)                              │││││
    │    %8  = :(Core.Int64)::Type{Int64}                                          │││││
    └───       goto 6 if not false                                                 │││││╻         overdub
    6 ┄─       Base.getfield(Cassette, :execute)                                   ││││││╻╷╷       recurse
    │          %7(1, %8)                                                           │││││││╻         macro expansion
    │    %12 = π (1, Int64)                                                        ││││││││┃│        execute
    └───       goto 7                                                              │││││╻         overdub
    7 ──       goto 8                                                              │││││
    8 ──       goto 9                                                              │││╻         overdub
    9 ── %16 = :(Base.pointerref)::Core.IntrinsicFunction                          │││
    └───       goto 10 if not false                                                │││╻         overdub
    10 ┄       Base.getfield(Cassette, :execute)                                   ││││╻╷╷       recurse
    │    %19 = %16(%2, %12, 1)::Any                                                │││││╻         macro expansion
    └───       goto 11                                                             │││╻         overdub
    11 ─       goto 12                                                             │││
    12 ─       goto 13                                                             │╻         overdub
    13 ─       return %19                                                          │
) => Float32

Here %19 should be inferred as a Float32, but isn't. It doesn't necessarily matter in this case since we inferred the output anyway, but in other cases this seems like an indicator that something broke.

@jrevels jrevels changed the title bad InstrinsicFunction inferrability is back on recent Julia master bad inferrability is back on recent Julia master May 31, 2018
@jrevels
Copy link
Collaborator Author

jrevels commented May 31, 2018

Don't have time to do a full bisect atm but:

julia> versioninfo()
Julia Version 0.7.0-DEV.5077
Commit 1d53232f7d (2018-05-11 20:37 UTC)
Platform Info:
  OS: macOS (x86_64-apple-darwin17.5.0)
  CPU: Intel(R) Core(TM) i7-7920HQ CPU @ 3.10GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.0 (ORCJIT, skylake)

julia> using Cassette

julia> Cassette.@context Ctx

julia> @code_warntype Cassette.overdub_execute(Ctx(), unsafe_load, pointer(Float32[]))
Variables:
  ctx<optimized out>
  args::Tuple{typeof(unsafe_load),Ptr{Float32}}
  i<optimized out>

Body:
  begin
      #= line 31 =#
      goto 4
      #= line 32 =#
      4:
      #= line 34 =#
      Core.SSAValue(11) = (Core.getfield)(args::Tuple{typeof(unsafe_load),Ptr{Float32}}, 2)::Ptr{Float32}
      # meta: location /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl overdub_recurse 157
      # meta: location /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl overdub_execute 31
      goto 11
      #= line 32 =#
      11:
      #= line 34 =#
      # meta: location /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl overdub_recurse 157
      # meta: location /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl overdub_execute 31
      goto 17
      #= line 32 =#
      17:
      #= line 34 =#
      # meta: location /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl overdub_recurse 171
      # meta: location /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl @generated body
      #= line 136 =#
      # meta: location /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl execution 9
      Core.SSAValue(61) = (pointerref)(Core.SSAValue(11), 1, 1)::Float32
      # meta: pop location
      goto 27
      # meta: pop location
      27:
      # meta: pop location
      29:
      # meta: pop locations (2)
      31:
      # meta: pop locations (2)
      33:
      #= line 37 =#
      return Core.SSAValue(61)
  end::Float32

@jrevels jrevels added the 0.1.0 label Jun 18, 2018
@jrevels
Copy link
Collaborator Author

jrevels commented Jun 19, 2018

Okay, I tracked this down to JuliaLang/julia@88f1712.

What that means is essentially that JuliaLang/julia#26981 was incomplete; I'm guessing that the SparseArray tests that were broken in that PR should've been passing as well.

From the MWE here, it's likely that the port of the varargs optimization over to the new optimizer is not hitting the InferenceResult cache correctly. I'll try to get a PR going to fix that (and hopefully speed up some Base code as well).

@jrevels
Copy link
Collaborator Author

jrevels commented Jun 19, 2018

So this is getting hard to debug without a fix for #47, so I'll tackle that first. As a reminder to myself, I can debug the cache by doing

using Cassette
Cassette.@context Ctx
f() = Cassette.recurse(Ctx(), unsafe_load, pointer(Float32[]))
p = Core.Compiler.Params(typemax(UInt))
Core.Compiler.typeinf_type(methods(f).ms[1], Tuple{typeof(f)}, Core.svec(), p) # now p.cache should be populated and introspectable

Also as a reminder to myself, here are some examples of the new optimizer's atypes transform for varargs methods before hitting the cache:

OLD atypes: Array{Any, (4,)}[
  Core.Compiler.Const(val=typeof(Core._apply)(), actual=false),
  Core.Compiler.Const(val=typeof(Base._maybe_reshape)(), actual=false),
  Tuple{Base.IndexLinear, Base.BitArray{2}},
  Tuple{Base.Slice{Base.OneTo{Int64}}, Int64}]
NEW atypes: Array{Any, (5,)}[
  Core.Compiler.Const(val=typeof(Base._maybe_reshape)(), actual=false),
  Core.Compiler.Const(val=Base.IndexLinear(), actual=false),
  Base.BitArray{2},
  Base.Slice{Base.OneTo{Int64}},
  Int64]

OLD atypes: Array{Any, (3,)}[
    Core.Compiler.Const(val=typeof(Core._apply)(), actual=false),
    Core.Compiler.Const(val=typeof(Base.index_lengths)(), actual=false),
    Tuple{Int64}]
NEW atypes: Array{Any, (2,)}[
    Core.Compiler.Const(val=typeof(Base.index_lengths)(), actual=false),
    Int64]

@jrevels
Copy link
Collaborator Author

jrevels commented Jul 9, 2018

Okay, from the example above, we can get:

julia> r = p.cache[findfirst(x -> x.args[1] === Core.Compiler.Const(Cassette.recurse, false) && x.vargs[1] === Core.Compiler.Const(Base.pointerref, false), p.cache)]
Core.Compiler.InferenceResult(MethodInstance for recurse(::Cassette.Context{nametype(Ctx),Nothing,Cassette.NoPass,Nothing,Nothing}, ::Core.IntrinsicFunction, ::Ptr{Float32}, ::Int64, ::Int64), Any[Core.Compiler.Const(Cassette.recurse, false), Core.Compiler.Const(Cassette.Context{nametype(Ctx),Nothing,Cassette.NoPass,Nothing,Nothing}(nametype(Ctx)(), nothing, Cassette.NoPass(), nothing, nothing), false), Tuple{Core.IntrinsicFunction,Ptr{Float32},Int64,Int64}], Any[Core.Compiler.Const(pointerref, false), Ptr{Float32}, Int64, Core.Compiler.Const(1, false)], Float32, CodeInfo(
224 1 ─      Base.getfield(Cassette, :execute)                                                  │╻╷ macro expansion
    │   %2 = getfield(%%recurse_arguments#377, 1)::Core.IntrinsicFunction                       ││
    │   %3 = getfield(%%recurse_arguments#377, 2)::Ptr{Float32}                                 ││
    │   %4 = getfield(%%recurse_arguments#377, 3)::Int64                                        ││
    │   %5 = getfield(%%recurse_arguments#377, 4)::Int64                                        ││
    │   %6 = %2(%3, %4, %5)::Any                                                                ││╻  execute
    └──      return %6                                                                          ││
))

So it looks like the vargs info is correct, but isn't getting properly used for %2 in the inferred result. Seems like a getfield constant propagation issue?

At the very least, it looks like cache_lookup is giving us the right result for r above (and I think we're calling it like this in the optimizer), so it's not that AFAICT:

julia> r === Core.Compiler.cache_lookup(r.linfo, Any[r.args[1:(end-1)]..., r.vargs...], p.cache)
true

@jrevels
Copy link
Collaborator Author

jrevels commented Jul 15, 2018

closed by JuliaLang/julia#28079

@jrevels jrevels closed this as completed Jul 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant