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

UnionAll triggers "ccall requires the compiler" #39

Closed
jumerckx opened this issue Jan 21, 2024 · 3 comments · Fixed by #46
Closed

UnionAll triggers "ccall requires the compiler" #39

jumerckx opened this issue Jan 21, 2024 · 3 comments · Fixed by #46

Comments

@jumerckx
Copy link

jumerckx commented Jan 21, 2024

EDIT:
I reduced this error further to:

using CassetteOverlay
@MethodTable MyTable
mypass = @overlaypass MyTable

mypass() do
    UnionAll(TypeVar(:T,Integer), Array{TypeVar(:T,Integer)})
end
ERROR: `ccall` requires the compiler
Stacktrace:
 [1] (::var"##MyTable#220")(::Type{UnionAll}, ::TypeVar, ::Type{Array{T<:Integer}})
   @ Main boot.jl:297
 [2] #8
   @ ~/masterthesis/mojo_in_julia/einsum.jl:111 [inlined]
 [3] (::var"##MyTable#220")(fargs::var"#8#9")
   @ Main ~/.julia/dev/CassetteOverlay/src/CassetteOverlay.jl:0
 [4] top-level scope
   @ ~/masterthesis/mojo_in_julia/einsum.jl:110

Comparing @code_lowered between without and with pass:

CodeInfo(
1 ─      nothing
│        nothing
│   %3 = Core.typeassert(v, Core.TypeVar)
│   %4 = $(Expr(:foreigncall, :(:jl_type_unionall), Any, svec(Any, Any), 0, :(:ccall), :(%3), :(t)))
└──      return %4
)
CodeInfo(
1 ─      (getfield)(fargs, 1)
│   @ [[...].jl:112 within `#21`]
│   %2 = Main.UnionAll
│   %3 = (#self#)(Main.TypeVar, :T, Main.Integer)
│   %4 = Main.Array
│   %5 = (#self#)(Main.TypeVar, :T, Main.Integer)
│   %6 = (#self#)(Core.apply_type, %4, %5)
│   %7 = (#self#)(%2, %3, %6)
└──      return %7
)

I logged some variables inside transform_stmt and found the :foreigncall expression is being transformed into
$(Expr(:foreigncall, :(:jl_type_unionall), Any, svec(Any, Any), 0, :(:ccall), :(%6), :(%3))).
I don't see what's different here compared to the example from this issue #14. Any help would be appreciated!


Original issue

The following example throws an error.

using CassetteOverlay
@MethodTable MyTable
mypass = @overlaypass MyTable

mypass() do
    sum([x for x in 1:3])
end
ERROR: `ccall` requires the compiler
Stacktrace:
 [1] (::var"##MyTable#233")(::Type{UnionAll}, ::TypeVar, ::Type{AbstractArray{var"#s127"<:Int64, 1}})
   @ Main boot.jl:297
 [2] collect
   @ ./array.jl:782 [inlined]
 [3] (::var"##MyTable#233")(::typeof(collect), ::Base.Generator{UnitRange{Int64}, typeof(identity)})
   @ Main ~/.julia/packages/CassetteOverlay/2hq0U/src/CassetteOverlay.jl:0
 [4] #33
   @ ./REPL[6]:2 [inlined]
 [5] (::var"##MyTable#233")(fargs::var"#33#34")
   @ Main ~/.julia/packages/CassetteOverlay/2hq0U/src/CassetteOverlay.jl:0
 [6] top-level scope
   @ REPL[6]:1

I can work around this by not using list comprehensions but was wondering if you know what's going on?
I'm using a somewhat recent Julia nightly, my versioninfo:

Julia Version 1.11.0-DEV.1231
Commit 0afa354c1f1 (2024-01-08 08:39 UTC)
Build Info:
  Official https://julialang.org/ release
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: 12 × Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
  WORD_SIZE: 64
  LLVM: libLLVM-15.0.7 (ORCJIT, skylake)
Threads: 1 default, 0 interactive, 1 GC (on 12 virtual cores)
Environment:
  JULIA_EDITOR = code
  JULIA_NUM_THREADS = 
@jumerckx jumerckx changed the title list comprehension triggers "ccall requires the compiler" UnionAll triggers "ccall requires the compiler" Mar 5, 2024
@maleadt
Copy link
Contributor

maleadt commented Mar 5, 2024

Hmm, not sure what's up here. The issue seems to be that we never compile the function (so ->invoke points to the interpreter), because we never inferred it, because jl_type_infer calling jl_typeinf_world returned nothing... However, calling inference in isolation generates an assertion failure:

using CassetteOverlay
@MethodTable MyTable
mypass = @overlaypass MyTable
world = Base.get_world_counter()

const CC = Core.Compiler
@inline function signature_type_by_tt(ft::Type, tt::Type)
    u = Base.unwrap_unionall(tt)
    return Base.rewrap_unionall(Tuple{ft, u.parameters...}, tt)
end
function methodinstance(ft::Type, tt::Type)
    sig = signature_type_by_tt(ft, tt)

    match, _ = CC._findsup(sig, nothing, world)
    match === nothing && throw(MethodError(ft, tt, world))

    mi = CC.specialize_method(match)

    return mi::CC.MethodInstance
end

ft = typeof(mypass)
tt = Tuple{Type{UnionAll}, TypeVar, Type{Array{TypeVar(:T,Integer)}}}
mi = methodinstance(ft, tt)::CC.MethodInstance

# inference fails, resulting in the interpreter firing
try
    mypass() do
        UnionAll(TypeVar(:T,Integer), Array{TypeVar(:T,Integer)})
    end
catch err
    Base.showerror(stdout, err)
    Base.show_backtrace(stdout, catch_backtrace())
end

println()

# inference asserts
@show @ccall jl_type_infer(mi::Any, world::Csize_t, false::Cint)::Any

So somehow doing UnionAll(TypeVar(:T,Integer), Array{TypeVar(:T,Integer)}) in a CassetteOverlay pass results in inference being invoked invalidly?

@jumerckx
Copy link
Author

jumerckx commented Apr 5, 2024

If anyone else is running into this issue, I worked around this by introducing an extra "primitive" to the overlaypass:

primitives = quote
	# ...
	@inline function (self::$PassName)(::Type{UnionAll}, v, t)
		return UnionAll(v, t)
	end
	# ...
end

This fixed the issue in the OP but I'm not sure if it is the best solution or doesn't introduce other nasty behaviour.

@aviatesk
Copy link
Member

The problem seems to be that the signature (::Main.var"##MyTable#230")(Type{UnionAll}, TypeVar, Type{Array{T<:Integer, N} where N}) is not an isdispatchtuple (especially since Type{Array{T<:Integer, N} where N} has an unbounded type variable within), so due to the guarantee of generated function soundness, inference does not run on the code generated by overlay_generator, and as Tim mentioned, it ends up being executed by the interpreter.
Dispatch with such free type variables are not necessarily something that should be supported, but as a simple solution, adding an overload for UnionAll might not be a bad option.

aviatesk added a commit that referenced this issue Jun 19, 2024
When dynamic dispatch occurs for the generated function and the dispatch
signature is `!isdispatchtuple` (e.g. since it includes a `Type`-object
argument with a free type variable), it will result in unintended code
generation, leading to issues like #39 and #45.
In this commit, the generated function is now not marked as
`:generated_only`, and it will simply fallback to original
implementations.

This fix introduces a regression where overlays will not occur for the
entire call graph of such `!isdispatchtuple` dynamic dispatches.
But since it would require a redesign of the runtime system to
completely resolve this issue, this regression seems to be acceptable
for now.
aviatesk added a commit that referenced this issue Jun 19, 2024
When dynamic dispatch occurs for the generated function and the dispatch
signature is `!isdispatchtuple` (e.g. since it includes a `Type`-object
argument with a free type variable), it will result in unintended code
generation, leading to issues like #39 and #45.
In this commit, the generated function is now not marked as
`:generated_only`, and it will simply fallback to original
implementations.

This fix introduces a regression where overlays will not occur for the
entire call graph of such `!isdispatchtuple` dynamic dispatches.
But since it would require a redesign of the runtime system to
completely resolve this issue, this regression seems to be acceptable
for now.
aviatesk added a commit that referenced this issue Jun 21, 2024
When dynamic dispatch occurs for the generated function and the dispatch
signature is `!isdispatchtuple` (e.g. since it includes a `Type`-object
argument with a free type variable), it will result in unintended code
generation, leading to issues like #39 and #45.
In this commit, the generated function is now not marked as
`:generated_only`, and it will simply fallback to original
implementations.

This fix introduces a regression where overlays will not occur for the
entire call graph of such `!isdispatchtuple` dynamic dispatches.
But since it would require a redesign of the runtime system to
completely resolve this issue, this regression seems to be acceptable
for now.
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 a pull request may close this issue.

3 participants