-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Get better type info from partially generated
functions
#31025
Conversation
1df7e97
to
b896fec
Compare
I found a crash here --- |
Yeah, I found the same crash last night. Was about to push a fix.
…On Mon, Feb 11, 2019, 10:28 Jeff Bezanson ***@***.***> wrote:
I found a crash here --- jl_code_for_staged assumes linfo->specTypes is a
tuple type, but now it can be a UnionAll type.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#31025 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABO1l6bBxSS0FSUIyVIEG1a2Ww5QSVM6ks5vMYwkgaJpZM4ay0xz>
.
|
Consider the following function: ``` julia> function foo(a, b) ntuple(i->(a+b; i), Val(4)) end foo (generic function with 1 method) ``` (In particular note that the return type of the closure does not depend on the types of `a` and b`). Unfortunately, prior to this change, inference was unable to determine the return type in this situation: ``` julia> code_typed(foo, Tuple{Any, Any}, trace=true) Refused to call generated function with non-concrete argument types ntuple(::getfield(Main, Symbol("##15#16")){_A,_B} where _B where _A, ::Val{4}) [GeneratedNotConcrete] 1-element Array{Any,1}: CodeInfo( 1 ─ %1 = Main.:(##15#16)::Const(##15#16, false) │ %2 = Core.typeof(a)::DataType │ %3 = Core.typeof(b)::DataType │ %4 = Core.apply_type(%1, %2, %3)::Type{##15#16{_A,_B}} where _B where _A │ %5 = %new(%4, a, b)::##15#16{_A,_B} where _B where _A │ %6 = Main.ntuple(%5, $(QuoteNode(Val{4}())))::Any └── return %6 ) => Any ``` Looking at the definition of ntuple https://github.com/JuliaLang/julia/blob/abb09f88804c4e74c752a66157e767c9b0f8945d/base/ntuple.jl#L45-L56 we see that it is a generated function an inference thus refuses to invoke it, unless it can prove the concrete type of *all* arguments to the function. As the above example illustrates, this restriction is more stringent than necessary. It is true that we cannot invoke generated functions on arbitrary abstract signatures (because we neither want to the user to have to be able to nor do we trust that users are able to preverse monotonicity - i.e. that the return type of the generated code will always be a subtype of the return type of a more abstract signature). However, if some piece of information is not used (the type of the passed function in this case), there is no problem with calling the generated function (since information that is unnused cannot possibly affect monotnicity). This PR allows us to recognize pieces of information that are *syntactically* unused, and call the generated functions, even if we do not have those pieces of information. As a result, we are now able to infer the return type of the above function: ``` julia> code_typed(foo, Tuple{Any, Any}) 1-element Array{Any,1}: CodeInfo( 1 ─ %1 = Main.:(##3#4)::Const(##3#4, false) │ %2 = Core.typeof(a)::DataType │ %3 = Core.typeof(b)::DataType │ %4 = Core.apply_type(%1, %2, %3)::Type{##3#4{_A,_B}} where _B where _A │ %5 = %new(%4, a, b)::##3#4{_A,_B} where _B where _A │ %6 = Main.ntuple(%5, $(QuoteNode(Val{4}())))::NTuple{4,Int64} └── return %6 ) => NTuple{4,Int64} ``` In particular, we use the new frontent `used` flags from the previous commit. One additional complication is that we want to accesss these flags without uncompressing the generator source, so we change the compression scheme to place the flags at a known location. Fixes #31004
b896fec
to
3bc4ea7
Compare
generator_mt = typeof(method.generator.gen).name.mt | ||
length(generator_mt) == 1 || return false | ||
|
||
generator_method = first(MethodList(generator_mt)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like it's introducing a bit of excess design debt, making it harder to fix #14919. The mt
field only exists as a implementation detail, and you shouldn't depend on it being meaning for anything. Also first(MethodList(generator_mt))
seems like a clear design smell...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So just use methods
from reflection.jl
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was concerned that that would try to do a method lookup that would be more expensive, than just doing this, which is O(1).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let the runtime worry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, will do.
This package was noted to fail on the 1.2 branch of Julia. The usage of `func_for_method_checked` was changed in JuliaLang/julia#31025 to also take an `sparams` argument. The old usage was deprecated but the function `depwarn` is not available in `Core.Compiler`. Using the `Base` version of this function will show the deprecation message instead of hard errorring.
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12 (cherry picked from commit 328dd57)
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12 (cherry picked from commit 328dd57)
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12 (cherry picked from commit 328dd57)
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12 (cherry picked from commit 328dd57)
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12 (cherry picked from commit 328dd57)
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12 (cherry picked from commit 328dd57)
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12 (cherry picked from commit 328dd57)
This code was introduced by me back in #31025 to speed up evaluation of generated functions that didn't make use of all of their arguments to make generation decisions. However, it neglected to take into account the possibility that the generator could be varargs. As a result, an unfortunate coincidence of an unused slot in the correct position could have allowed expansion of generators that were not supposed to be expandable. This can cause incorrect inference with all the usual consequences. However, fortunately this coincidence appears to be pretty rare. Fixes JuliaDebug/CassetteOverlay.jl#12 (cherry picked from commit 328dd57)
Consider the following function:
(In particular note that the return type of the closure does not depend on the types
of
a
and b`). Unfortunately, prior to this change, inference was unable to determinethe return type in this situation:
Looking at the definition of ntuple
julia/base/ntuple.jl
Lines 45 to 56 in abb09f8
we see that it is a generated function an inference thus refuses to invoke it,
unless it can prove the concrete type of all arguments to the function. As
the above example illustrates, this restriction is more stringent than necessary.
It is true that we cannot invoke generated functions on arbitrary abstract
signatures (because we neither want to the user to have to be able to nor
do we trust that users are able to preverse monotonicity - i.e. that the return
type of the generated code will always be a subtype of the return type of a more
abstract signature).
However, if some piece of information is not used (the type of the passed function
in this case), there is no problem with calling the generated function (since
information that is unnused cannot possibly affect monotnicity).
This PR allows us to recognize pieces of information that are syntactically unused,
and call the generated functions, even if we do not have those pieces of information.
As a result, we are now able to infer the return type of the above function:
In particular, we use the new frontent
used
flags from the previous commit.One additional complication is that we want to accesss these flags without
uncompressing the generator source, so we change the compression scheme to
place the flags at a known location.
Fixes #31004