Avoid broken kind type handling when compiling isa
#27736
Conversation
src/cgutils.cpp
Outdated
| // | ||
| // Optional<bool> known_isa; | ||
| // if (x.constant) | ||
| // known_isa = jl_isa(x.constant, type); |
There was a problem hiding this comment.
Isn't this case correct? The result of an actual call to jl_isa seems like it should be correct by definition. I agree this optimization is probably redundant though.
There was a problem hiding this comment.
Yeah, makes sense. I'll uncomment this branch out and see if it still passes tests (I assume it will).
base/tuple.jl
Outdated
| (convert(tuple_type_head(T), y[1]), _totuple(tuple_type_tail(T), itr, y[2])...) | ||
| end | ||
|
|
||
| _totuple(::Type{Tuple{Vararg{E}}}, itr, s...) where {E} = (collect(E, Iterators.rest(itr,s...))...,) |
There was a problem hiding this comment.
This definition could be combined with the next by adding an @isdefined(E) check.
base/essentials.jl
Outdated
| convert(::Type{T}, x::T) where {T<:Tuple{Any, Vararg{Any}}} = x | ||
| convert(::Type{Tuple{}}, x::Tuple{Any, Vararg{Any}}) = throw(MethodError(convert, (Tuple{}, x))) | ||
| convert(::Type{T}, x::Tuple{Any, Vararg{Any}}) where {T<:Tuple} = | ||
| (convert(tuple_type_head(T), x[1]), convert(tuple_type_tail(T), tail(x))...) |
There was a problem hiding this comment.
Do you have test cases where these fail?
There was a problem hiding this comment.
The actual problem that I'm trying to workaround with this PR's fieldtype-based implementation is e.g.:
julia> using Cassette
julia> Cassette.@context Ctx
julia> Cassette.@overdub(Ctx(), convert(Tuple{Vararg{Float64}}, (1,2)))
ERROR: type DataType has no field body
Stacktrace:
[1] execution at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:9 [inlined]
[2] macro expansion at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:171 [inlined]
[3] overdub_recurse at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:201 [inlined]
[4] overdub_execute at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:35 [inlined]
[5] getproperty at ./sysimg.jl:15 [inlined]
[6] overdub_recurse(::Cassette.Context{getfield(Main, Symbol("##CtxName#350")),Nothing,Cassette.NoPass,Nothing,Nothing}, ::typeof(getproperty), ::Type{Vararg{Float64,N} where N}, ::Symbol) at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:0
[7] overdub_execute at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:35 [inlined]
[8] unwrap_unionall at ./essentials.jl:179 [inlined]
[9] overdub_recurse(::Cassette.Context{getfield(Main, Symbol("##CtxName#350")),Nothing,Cassette.NoPass,Nothing,Nothing}, ::typeof(Base.unwrap_unionall), ::Type{Vararg{Float64,N} where N}) at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:0
[10] overdub_execute at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:35 [inlined]
[11] unwrapva at ./essentials.jl:224 [inlined]
[12] overdub_recurse(::Cassette.Context{getfield(Main, Symbol("##CtxName#350")),Nothing,Cassette.NoPass,Nothing,Nothing}, ::typeof(Base.unwrapva), ::Type{Vararg{Float64,N} where N}) at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:0
[13] overdub_execute at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:35 [inlined]
[14] tuple_type_head at ./essentials.jl:154 [inlined]
[15] overdub_recurse(::Cassette.Context{getfield(Main, Symbol("##CtxName#350")),Nothing,Cassette.NoPass,Nothing,Nothing}, ::typeof(Base.tuple_type_head), ::Type{Tuple{Vararg{Float64,N} where N}}) at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:0
[16] overdub_execute at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:35 [inlined]
[17] convert at ./essentials.jl:248 [inlined]
[18] overdub_recurse(::Cassette.Context{getfield(Main, Symbol("##CtxName#350")),Nothing,Cassette.NoPass,Nothing,Nothing}, ::typeof(convert), ::Type{Tuple{Vararg{Float64,N} where N}}, ::Tuple{Int64,Int64}) at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:0
[19] overdub_execute at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:35 [inlined]
[20] #19 at /Users/jarrettrevels/.julia/dev/Cassette/src/macros.jl:94 [inlined]
[21] overdub_recurse(::Cassette.Context{getfield(Main, Symbol("##CtxName#350")),Nothing,Cassette.NoPass,Nothing,Nothing}, ::getfield(Main, Symbol("##19#20"))) at /Users/jarrettrevels/.julia/dev/Cassette/src/overdub.jl:0
[22] top-level scope at none:0which seems to be from tuple_type_head/tuple_type_tail hitting some kindtype subtyping/inference issues that I don't fully understand. With this PR (EDIT: specifically, with this change to convert), I get the correct answer e.g.
julia> Cassette.@overdub(Ctx(), convert(Tuple{Vararg{Float64}}, (1,2)))
(1.0, 2.0)
base/essentials.jl
Outdated
| @@ -141,33 +141,6 @@ end | |||
| argtail(x, rest...) = rest | |||
| tail(x::Tuple) = argtail(x...) | |||
|
|
|||
| # TODO: a better / more infer-able definition would pehaps be | |||
| # tuple_type_head(T::Type) = fieldtype(T::Type{<:Tuple}, 1) | |||
There was a problem hiding this comment.
I think we should instate this definition of tuple_type_head, and add a correct implementation of tuple_type_tail.
There was a problem hiding this comment.
Hmm....what would a correct implementation of tuple_type_tail look like when we have covariance + diagonal constraints?
There was a problem hiding this comment.
Well, you seem content with fieldtype((Tuple{T,T} where T<:AbstractFloat), 1) == AbstractFloat, so it might not be a problem.
tuple_type_tail(Tuple{T,T} where T) == Tuple{T} where T seems correct to me. Do you have a counterexample?
There was a problem hiding this comment.
Here's an example that seems wrong to me for master's tuple_type_tail:
julia> t = (Val(Any), 1, 1.0)
(Val{Any}(), 1, 1.0)
julia> typeof(t) <: Tuple{Val{T},T,T} where T
true
julia> typeof(Base.tail(t)) <: Base.tuple_type_tail(Tuple{Val{T},T,T} where T)
falseThat's what I meant by covariance + diagonal constraints mucking things up, but I could totally be ignorant about the correct meaning of tuple_type_tail - apologies if so!
Well, you seem content with fieldtype((Tuple{T,T} where T<:AbstractFloat), 1) == AbstractFloat, so it might not be a problem.
Yeah, @vtjnash told me fieldtype usually gets the right answers in these cases, which is why I replaced all the usages of tuple_type_head/tuple_type_tail with it. I'm just not sure how to use fieldtype to compute tuple_type_tail in general e.g. on vatuple types with unconstrained lengths.
base/iterators.jl
Outdated
| if isvatuple(T) | ||
| throw(ArgumentError("Cannot compute IteratorSize for ProductIterator{$T}")) | ||
| end | ||
| return reduce(prod_iteratorsize, HasShape{0}(), ntuple(i -> IteratorSize(fieldtype(T, i))), fieldcount(T)) |
There was a problem hiding this comment.
Indeed, using ntuple and reduce in cases like this is pretty hard on the compiler. We should try to make tuple_type_tail work, or manually recur over field indices.
198b618 to
6e0120d
Compare
|
I've removed the more controversial tuple stuff from this PR, leaving only the |
isa and in tuple handling codeisa
|
So improving the julia> issubstrict(x, y) = (x <: y) && !(y <: x)
issubstrict (generic function with 1 method)
julia> f() = issubstrict(Int where T, Integer where T<:Integer)
f (generic function with 1 method)
# the SSA values that should be inferred as Type{T}s are inferred as Ts!!!
# but it doesn't matter because the `isa` tfunc still had a correct fallback of `Bool`
julia> code_typed(f, optimize = false)
1-element Array{Any,1}:
CodeInfo(
1 1 ─ T@_3 = (Core.TypeVar)(:T) │
│ %2 = Core.UnionAll(:(_3::Core.Compiler.PartialTypeVar(T, true, true)), Main.Int)::Int64
│ T@_2 = (Core.TypeVar)(:T, Main.Integer) │
│ %4 = Core.UnionAll(:(_2::Core.Compiler.PartialTypeVar(T<:Integer, true, true)), Main.Integer)::Integer
│ %5 = Main.issubstrict(%2, %4)::Bool │
└── return %5 │
) => Bool |
|
Okay, narrowed down the latest bug to this test case: @noinline function g(f, a::T, ::Type{S} = Any) where {T, S}
cf = @cfunction $f Ref{T} (Ref{T},)
GC.gc()
@assert cf === Base.cconvert(Ptr{Cvoid}, cf)
GC.@preserve cf begin
fptr = Base.unsafe_convert(Ptr{Cvoid}, cf)
b = ccall(fptr, Ref{T}, (Ref{T},), a)
end
end
g(identity, 1)Note that you may need to try this several times (restarting Julia each time) to see errors here - sometimes it just outputs julia> g(identity, 1)
ERROR: TypeError: in cfunction, in cfunction, expected Union{LinearAlgebra.Adjoint{T,Array{T,1}}, LinearAlgebra.RowVector{T,Array{T,1}}, LinearAlgebra.Transpose{T,Array{T,1}}, LinearAlgebra.AbstractTriangular{T,A} where A<:(Array{T,2} where T), LinearAlgebra.Hermitian{T,A} where A<:(Array{T,2} where T), LinearAlgebra.Symmetric{T,A} where A<:(Array{T,2} where T)}, got Int64
Stacktrace:
[1] macro expansion at ./REPL[1]:89 [inlined]
[2] macro expansion at ./gcutils.jl:87 [inlined]
[3] g(::Function, ::Int64, ::Type{Any}) at ./REPL[1]:5
[4] g(::Function, ::Int64) at ./REPL[1]:2
[5] top-level scope at none:0julia> g(identity, 1)
ERROR: TypeError: in cfunction, in cfunction, expected Core.Compiler.UseRef(nothing, 0), got Int64
Stacktrace:
[1] macro expansion at ./REPL[1]:89 [inlined]
[2] macro expansion at ./gcutils.jl:87 [inlined]
[3] g(::Function, ::Int64, ::Type{Any}) at ./REPL[1]:5
[4] g(::Function, ::Int64) at ./REPL[1]:2
[5] top-level scope at none:0julia> g(identity, 1)
ERROR: TypeError: in cfunction, in cfunction, expected Core.Compiler.UseRef(Core.UpsilonNode(Core.Compiler.Argument(2)), 2), got Int64
Stacktrace:
[1] macro expansion at ./REPL[1]:89 [inlined]
[2] macro expansion at ./gcutils.jl:87 [inlined]
[3] g(::Function, ::Int64, ::Type{Any}) at ./REPL[1]:5
[4] g(::Function, ::Int64) at ./REPL[1]:2
[5] top-level scope at none:0julia> g(identity, 1)
ERROR: TypeError: in cfunction, in cfunction, expected Core.Compiler.Pair{Int64,Any}(2, :((Base.arraylen)(Core.SSAValue(1)))), got Int64
Stacktrace:
[1] macro expansion at ./REPL[1]:89 [inlined]
[2] macro expansion at ./gcutils.jl:87 [inlined]
[3] g(::Function, ::Int64, ::Type{Any}) at ./REPL[1]:5
[4] g(::Function, ::Int64) at ./REPL[1]:2
[5] top-level scope at none:0...etc. |
|
Bump, any thoughts on the above? This PR works around one of the last correctness bugs that kills Cassette, if we can get it merged there will only be one last perf issue blocking the closure of #25492, and it would be wonderful if we could close that before v0.7 is released. |
|
Looks like a missing GC root: Looks like |
src/cgutils.cpp
Outdated
| // else if (jl_subtype(x.typ, type)) | ||
| // known_isa = true; | ||
| // else { | ||
| // intersected_type = jl_type_intersection(x.typ, type); |
There was a problem hiding this comment.
Is it likely/possible that this bug was caused by commenting this branch out? It seems like commenting this out might change the result of the jl_is_concrete_type(intersected_type) check below for non-constants.
I'm making a test build with this uncommented now, but if anybody happens to know what's actually going on I'd appreciate it...
There was a problem hiding this comment.
Nope, doesn't fix the MWE above. I did push the commit to uncomment the branch, though, since it seems more correct to my naive eyes and doesn't hurt the test cases I'm trying to fix with this PR.
src/cgutils.cpp
Outdated
| else { | ||
| else if (jl_subtype(x.typ, type)) { | ||
| // known_isa = true; | ||
| } else { |
There was a problem hiding this comment.
Okay, the minimal change that my tests hinge on seems to be here. Here is a gist of the tests I'm using to check this PR.
Before commenting out known_isa = true, some of the tests fail.
After commenting out known_isa = true, all of them pass except for the g(identity, 1) MWE reported earlier.
Does anybody know (or can help figure out) how to get all of these tests passing so this can be merged?
02b9db1 to
cc82c07
Compare
|
Rebased to avoid going stale. Is there anything extra I should be doing to get responses to the questions in this PR? If I should do anything before requesting more attention on this, please tell me and I'll do it. Sorry for the repeat bumping, I just fear that this PR will end up like #24852, which was waiting on review for months that never came. |
|
Probably you know this, but maybe it is interesting. while after the patch, Also looking at with respect to what seems a recursive call of boxed values : Probably is totally unrelated, but the heavy reliance on |
cc82c07 to
559090a
Compare
|
I cannot reproduce on MacOS. I tried the test case on more than 30 freshly launched REPLs and it didn't error a single time. |
|
Gah tests are failing now on the merge commit with master due to #27912 |
…oken Justification for allowing this test to remain broken for now: - benchmarking the expression (including downstream toy calculations on the output, e.g. broadcast sin) using BenchmarkTools reveals no actual performance difference - inference returning an optimal result before was probably reliant on the broken subtyping behavior; correctness >>> performance - inference is still returning a fairly tightly bounded, correct Union type
8089637 to
8705622
Compare
|
Man, this was so close. Finally got tests passing, and then got sniped at the finish line by changes to master... I've rebased and will investigate. |
|
Which tests are you seeing fail? |
|
The ones added in #27912: With this PR, it seems they're behaving the way they would before #27912, i.e. All other tests are passing. |
|
Are you sure you didn't just accidentally forget to rebuild the sysimage? |
I'm not sure of anything these days :p failures are happening on CI here: https://freebsdci.julialang.org/#/builders/1/builds/11024/steps/8/logs/stdio |
|
Errr actually it looks like only |
|
Reason might be because here: while on master: (where |
|
Yep, that'd do it (though of course |
base/compiler/tfuncs.jl
Outdated
| @@ -391,15 +394,17 @@ add_tfunc(isa, 2, 2, | |||
| if t === Bottom | |||
| return Const(false) | |||
There was a problem hiding this comment.
though of course Union{} would be a better answer from the tfunc here
Changing this to return Bottom seems to fix g9765(), and I think is correct based on this PR's instanceof_tfunc implementation. Unsure, though, this stuff hurts my brain 😛
I'll run tests and see how it goes.
EDIT:
Actually, that's maybe too naive - now trying
diff --git a/base/compiler/tfuncs.jl b/base/compiler/tfuncs.jl
index 22ed40a662..4dca7c05b9 100644
--- a/base/compiler/tfuncs.jl
+++ b/base/compiler/tfuncs.jl
@@ -389,10 +389,17 @@ add_tfunc(typeassert, 2, 2,
end, 4)
add_tfunc(isa, 2, 2,
function (@nospecialize(v), @nospecialize(t))
+ _t = t
t, isexact = instanceof_tfunc(t)
if !has_free_typevars(t)
if t === Bottom
- return Const(false)
+ # we make this check because `t` could be `Bottom` when `_t` is e.g.
+ # `Type{Union{}}`, in which case we want to return `Const(false)`
+ if typeintersect(_t, Type) === Bottom && _t !== typeof(Bottom)
+ return Bottom
+ else
+ return Const(false)
+ end
elseif v ⊑ t
if isexact && isnotbrokensubtype(v, t)
return Const(true)
There was a problem hiding this comment.
That doesn't look quite right. Here's the patch I'm using over in #27972 which needed this same fix:
diff --git a/base/compiler/tfuncs.jl b/base/compiler/tfuncs.jl
index 161e26e984..a7b9d5fa58 100644
--- a/base/compiler/tfuncs.jl
+++ b/base/compiler/tfuncs.jl
@@ -388,12 +390,18 @@ add_tfunc(typeassert, 2, 2,
return typeintersect(v, t)
end, 4)
add_tfunc(isa, 2, 2,
- function (@nospecialize(v), @nospecialize(t))
- t, isexact = instanceof_tfunc(t)
+ function (@nospecialize(v), @nospecialize(tt))
+ t, isexact = instanceof_tfunc(tt)
+ if t === Bottom
+ # check if t could be equivalent to typeof(Bottom), since that's valid in `isa`, but the set of `v` is empty
+ # if `t` cannot have instances, it's also invalid on the RHS of isa
+ if typeintersect(widenconst(tt), Type) === Union{}
+ return Union{}
+ end
+ return Const(false)
+ end
if !has_free_typevars(t)
- if t === Bottom
- return Const(false)
- elseif v ⊑ t
+ if v ⊑ t
if isexact
return Const(true)
end
@@ -401,7 +409,7 @@ add_tfunc(isa, 2, 2,
# this tests for knowledge of a leaftype appearing on the LHS
# (ensuring the isa is precise)
return Const(false)
- elseif isexact && typeintersect(v, t) === Bottom
+ elseif typeintersect(v, t) === Bottom
if !iskindtype(v) #= subtyping currently intentionally answers this query incorrectly for kinds =#
return Const(false)
end
diff --git a/test/compiler/compiler.jl b/test/compiler/compiler.jl
index 9c6d32d529..e22b12f216 100644
--- a/test/compiler/compiler.jl
+++ b/test/compiler/compiler.jl
@@ -1189,7 +1189,7 @@ let isa_tfunc = Core.Compiler.T_FFUNC_VAL[
@test isa_tfunc(typeof(Union{}), Const(Int)) === Const(false) # any result is ok
@test isa_tfunc(typeof(Union{}), Const(Union{})) === Const(false)
@test isa_tfunc(typeof(Union{}), typeof(Union{})) === Const(false)
- @test isa_tfunc(typeof(Union{}), Union{}) === Const(false) # any result is ok
+ @test isa_tfunc(typeof(Union{}), Union{}) === Union{}
@test isa_tfunc(typeof(Union{}), Type{typeof(Union{})}) === Const(true)
@test isa_tfunc(typeof(Union{}), Const(typeof(Union{}))) === Const(true)
let c = Conditional(Core.SlotNumber(0), Const(Union{}), Const(Union{}))
@@ -1204,7 +1204,7 @@ let isa_tfunc = Core.Compiler.T_FFUNC_VAL[
@test isa_tfunc(Val{1}, Type{Val{T}} where T) === Bool
@test isa_tfunc(Val{1}, DataType) === Bool
@test isa_tfunc(Any, Const(Any)) === Const(true)
- @test isa_tfunc(Any, Union{}) === Const(false) # any result is ok
+ @test isa_tfunc(Any, Union{}) === Union{}
@test isa_tfunc(Any, Type{Union{}}) === Const(false)
@test isa_tfunc(Union{Int64, Float64}, Type{Real}) === Const(true)
@test isa_tfunc(Union{Int64, Float64}, Type{Integer}) === BoolThere was a problem hiding this comment.
Ah, nice - this fixes the isa_tfunc(typeof(Union{}), Const(Union{})) that I broke with my version of the fix (it should indeed return Const(false), but my patch was making it return Union{}.
Thanks!
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan |
aa48848 to
223b797
Compare
|
Perf changes are spurious, the one CI failure is from curl failing to download something. |
This fixes #27078, JuliaLabs/Cassette.jl#42, and some other bugs with tuples.
I'm not at all a fan of my replacements for the tuple handling code here - I just need to get the aforementioned bugs fixed. It should be "more correct" than it was before (modulo any bugs I introduced...), but it feels like it's going to be slower w.r.t. to compilation, runtime, or both. I haven't done benchmarking yet, though. I would be extremely grateful for some tips on faster/cleaner alternatives (as long as they don't reintroduce the bugs I'm trying to fix). This might be another case in favor of making
ntuplean intrinsic...EDIT: see comment below, moved the tuple changes out of this PR@nanosoldier
runbenchmarks(ALL, vs = ":master")