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

Better varargs are actually worse #31

Closed
maleadt opened this issue Feb 8, 2018 · 6 comments
Closed

Better varargs are actually worse #31

maleadt opened this issue Feb 8, 2018 · 6 comments

Comments

@maleadt
Copy link
Contributor

maleadt commented Feb 8, 2018

After ecbe4d9 the following example fails to inline calls to overdub:

using Cassette
Cassette.@context Ctx

code_warntype(Cassette.overdub(Ctx, unsafe_load), Tuple{Ptr{Float32}})

Before:

Body:
  begin
     ...
      Core.SSAValue(491) = (pointerref)(x1::Ptr{Float32}, Core.SSAValue(316), 1)::Any
      ...
  end::Any

After:

Body:
  begin
      ...
      Core.SSAValue(32) = $(Expr(:invoke, MethodInstance for (::Cassette.Overdub{Cassette.Execute,typeof(unsafe_load),Cassette.Settings{Ctx{0xf0c703cd62596463},Cassette.Unused,0x00000000000063ef,false,Cassette.Unused}})(::Ptr{Float32}, ::Int64), :($(QuoteNode(Overdub{Execute}(Ctx, unsafe_load)))), :(x1), 1))::Any
      ...
  end::Any

For reference, the non-overdubbed IR:

Body:
  begin
      # meta: location pointer.jl unsafe_load 105
      Core.SSAValue(2) = (Base.pointerref)(p::Ptr{Float32}, 1, 1)::Float32
      # meta: pop location
      return Core.SSAValue(2)
  end::Float32

cc @cfoket

@jrevels
Copy link
Collaborator

jrevels commented Feb 8, 2018

That issue name 😂 Yeah, the "better" in that commit referred to a correctness improvement rather than a performance improvement 😛

Thanks for finding/cataloging these regressions, it's extremely useful.

@jrevels
Copy link
Collaborator

jrevels commented Apr 18, 2018

With #40 + JuliaLang/julia#26826 + a hack to also ignore Overdub{Execute} in the recursion limiting heuristic [1], this goes back to your "before" case...which is still inferring as Any.

Sigh.

[1] This is a separate hack from the spoofing we're using to ignore Overdub{Transform}...we haven't decided how to do it in a not hacky way yet.

@jrevels
Copy link
Collaborator

jrevels commented Apr 19, 2018

Okay, JuliaLang/julia#26826 now implements the recursion limiting spoofing in a way that fixes the problem we were hacking around, so with #40 and JuliaLang/julia#26826 this becomes:

julia> code_warntype(Cassette.overdub(Ctx, unsafe_load), Tuple{Ptr{Float32}})
Variables:
  o<optimized out>
  x1::Ptr{Float32}
  i<optimized out>

Body:
  begin
      Core.SSAValue(119) = (typeassert)(1, Int64)::Int64
      Core.SSAValue(192) = (pointerref)(x1::Ptr{Float32}, Core.SSAValue(119), 1)::Any
      return Core.SSAValue(192)
  end::Any

@maleadt
Copy link
Contributor Author

maleadt commented Apr 19, 2018

Yay, looking good. Any idea which limit/deficiency prevents pointerref from being inferred?

@jrevels
Copy link
Collaborator

jrevels commented Apr 20, 2018

Any idea which limit/deficiency prevents pointerref from being inferred?

The problem was that intrinsics don't each have their own type - they're all of type Core.IntrinsicFunction, so inference relies on constant propagation in order to infer calls to them. However, Cassette's splatting was thwarting the compiler's ability to constant propagate. JuliaLang/julia#26826 enables varargs constant propagation (and adds another patch on top of the recursion limiting heuristic), thus allowing this case to be inferred.

Leveraging this required refactoring Cassette's overdub implementation a bit (#41), because storing the function in an intermediate Overdub struct was also getting in the way of constant propagation (and is no longer necessary, anyway, since I've been told that other compiler patches have since fixed the problems that struct was hacking around).

@jrevels
Copy link
Collaborator

jrevels commented May 10, 2018

closed by #41

@jrevels jrevels closed this as completed May 10, 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

2 participants