-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Support resolving dispatch in apply_iterate
for variable-length containers (Tuple{Vararg{T}}
and Vector{T}
)
#57830
Comments
Thanks for the detailed description! I'll take a look at this. |
apply_iterate
for variable-length containers (Tuple{Vararg{T}}
and Vector{T}
)apply_iterate
for variable-length containers (Tuple{Vararg{T}}
and Vector{T}
)
Why may we annotate a It seems to me like the dynamic annotation implies that we may eventually have a dynamic dispatch at runtime (to |
Yes, that's all correct 👍 The Also FWIW, erroring at runtime does not count as |
BTW even if the dispatch to the built-in were dynamic, we wouldn't care since the runtime is guaranteed to provide all built-in implementations. The issue is that we lose the dispatch edge into user code, which we try to aggressively trim
|
Could have |
ObservationsAs I understand it, for
while for the case of a single iterable, specific apply targets/iterable type combinations (only involving The fact that iteration is hardcoded in the built-in is a potential source of bugs: julia> struct MyType end
julia> Base.iterate(::Vector{MyType}) = nothing # don't iterate the vector
julia> x = MyType()
MyType()
julia> args = MyType[]; for el in [x, x] push!(args, el) end; (args...,)
()
julia> f() = 1
f (generic function with 1 method)
julia> f(a, b) = 2
f (generic function with 2 methods)
julia> f([x, x]...) # should call `f()` but it calls `f(x, x)` instead
2 Conceptually we have two function calls per iterable argument: And a final function call: Possible designThe most straightforward case is to have a single iterable argument. Following the argument convention of To support the multi-argument case, we may instead define it as That would allow us to remove all hardcoded cases in favor of explicit (and optionally resolved) callables. This would be more correct in the sense of respecting user overloads and would address the current issue, however I'm not sure about the performance implications (notably in the interpreted case if we remove the hardcoded cases where we can't resolve calls to a Any thoughts/ideas regarding this design (or alternative proposals)? |
That sounds OK to me, although it does restrict us to perfectly type-stable (and well-inferred) My only argument for an alternative is that if we "just inlined" this to an equivalent implementation (adding loops, etc.) we'd have more freedom to perform union-splitting, etc. for the various |
There are two other possibilities also perhaps worth mentioning:
|
Right now,
--trim
has a very hard time compiling code like this:As
code_typed
helpfully points out, this has a dynamic call even after optimizations:This is a bit unfortunate, since inference is pretty good at exploring the calls that this
_apply_iterate
will actually perform. The problem is thatTuple
is not allowed to have an unknown length forinlining
to transform this to a "dispatch-resolved" form.Similarly, we have a very hard time with:
This should be a very simple operation, but the variable-length again makes
inlining
inapplicable.Finally, there are cases that we support just fine, but only because we "cheat" the dispatch:
This only works in
--trim
because of somewhat embarrassing hard-coded cases inbuiltins.c
that don't respect user overloads / dispatch semantics.I suspect the solution might be to introduce a "dispatch-resolved" version of
Core._apply_iterate
. However, it might also be possible to relax the restrictions in theinlining
pass, if there is a legal transform possible with the existing "dispatch-resolved" primitives, i.e.,Expr(:invoke, ...)
.Either way, we'd like to find a way to have the optimizer resolve all these dispatches, instead of leaving them un-annotated and hard-coding some of their results.
The text was updated successfully, but these errors were encountered: