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

Different type-widening for indirect vs. direct recursion #45759

Closed
martinholters opened this issue Jun 20, 2022 · 8 comments · Fixed by #50696
Closed

Different type-widening for indirect vs. direct recursion #45759

martinholters opened this issue Jun 20, 2022 · 8 comments · Fixed by #50696
Labels
compiler:inference Type inference

Comments

@martinholters
Copy link
Member

This is good:

julia> f(x::Tuple{Any,Vararg}) = x[1] + f(x[2:end])
f (generic function with 1 method)

julia> f(x::Tuple{}) = 0
f (generic function with 2 methods)

julia> only(Base.return_types(f, Tuple{Tuple{Int,Int,Int,Int,Int,Int,Int}}))
Int64

However, when introducing a level of indirection, inference widens more aggressively when detecting the recursion and fails:

julia> g(x::Tuple{Any,Vararg}) = x[1] + _g(x[2:end])
g (generic function with 1 method)

julia> g(x::Tuple{}) = 0
g (generic function with 2 methods)

julia> _g(x) = g(x)
_g (generic function with 1 method)

julia> only(Base.return_types(g, Tuple{Tuple{Int,Int,Int,Int,Int,Int,Int}}))
Any

This is particularly unfortunate as this kind of indirection is implicitly introduced when adding kwargs:

julia> h(x::Tuple{Any,Vararg}; kwargs...) = x[1] + h(x[2:end]; kwargs...)
h (generic function with 1 method)

julia> h(x::Tuple{}; kwargs...) = 0
h (generic function with 2 methods)

julia> only(Base.return_types(h, Tuple{Tuple{Int,Int,Int,Int,Int,Int,Int}}))
Any

A manifestation of this effect is #45715.

We deliberately distinguish the cases of direct vs. indirect recursion:

if method === sv.linfo.def
# Under direct self-recursion, permit much greater use of reducers.
# here we assume that complexity(specTypes) :>= complexity(sig)
comparison = sv.linfo.specTypes
l_comparison = length((unwrap_unionall(comparison)::DataType).parameters)
spec_len = max(spec_len, l_comparison)
else
comparison = method.sig
end

I'd argue that we shouldn't do this and instead, treat both cases equal. As a first trial balloon, I did

--- a/base/compiler/abstractinterpretation.jl
+++ b/base/compiler/abstractinterpretation.jl
@@ -542,7 +542,7 @@ function abstract_call_method(interp::AbstractInterpreter, method::Method, @nosp
             l_comparison = length((unwrap_unionall(comparison)::DataType).parameters)
             spec_len = max(spec_len, l_comparison)
         else
-            comparison = method.sig
+            comparison = topmost.linfo.specTypes
         end
 
         if isdefined(method, :recursion_relation)

That indeed lets above cases all be inferred as Int (and also fixes #45715). However, one @inferred test in the broadcast tests then fails. Although I didn't really know what I was doing there, I was not expecting this to worsen inference results, but maybe it's now triggering some other edge case? Anyway, feedback from @vtjnash on why these two cases are treated differently and whether it's a good idea to try to unify them would be welcome.

@schlichtanders
Copy link

Two of my packages also run into this issue. There are mainly of functional nature, where recursion is a typical tool.
It would be great if double recursion could be as good as single recursion

@schlichtanders
Copy link

Just tried to define recursion_relation here, and indeed it works 🙂
(first time I see that this workaround actually works - thank you for the convincing simple example)

Here the g exampleusing singlewhich` calls to grasp the method definitions

julia> g(x::Tuple{Any,Vararg}) = x[1] + _g(x[2:end])
g (generic function with 1 method)

julia> g(x::Tuple{}) = 0
g (generic function with 2 methods)

julia> _g(x) = g(x)
_g (generic function with 1 method)

julia> which(g, Tuple{Tuple{Any, Vararg}}).recursion_relation = (@nospecialize(_...)) -> true
#1 (generic function with 1 method)

julia> which(_g, Tuple{Any}).recursion_relation = (@nospecialize(_...)) -> true
#3 (generic function with 1 method)

julia> only(Base.return_types(g, Tuple{Tuple{Int,Int,Int,Int,Int,Int,Int}}))
Int64

Here the h example with kwfunction using methods to loop through all method definitions

julia> h(x::Tuple{Any,Vararg}; kwargs...) = x[1] + h(x[2:end]; kwargs...)
h (generic function with 1 method)

julia> h(x::Tuple{}; kwargs...) = 0
h (generic function with 2 methods)

julia> for m in methods(h)
           m.recursion_relation = (@nospecialize(_...)) -> true
       end

julia> for kwm in methods(Core.kwfunc(h))
           kwm.recursion_relation = (@nospecialize(_...)) -> true
       end
       
julia> only(Base.return_types(h, Tuple{Tuple{Int,Int,Int,Int,Int,Int,Int}}))
Int64

@vtjnash
Copy link
Member

vtjnash commented Dec 8, 2022

Manually mutating the fields of an internal object such as Method is undefined behavior, and should not be done too casually

@StefanKarpinski
Copy link
Member

By "not done too casually" what is really meant is, it might work today, but it not guaranteed to continue working in the future, so do at the risk of having to add some ugly version checks to support a future version of Julia. Which is not the end of the world, but fair warning seems appropriate.

@vtjnash
Copy link
Member

vtjnash commented Dec 9, 2022

And also it might blow up the compiler in really weird unexpected and basically inexplicable ways, since you are deleting one of the initial assumptions that is required to show that compilation will eventually converge to a fixed point solution.

@StefanKarpinski
Copy link
Member

Ok, that's substantially worse and probably not recommended even on a version of Julia that it happens to work on in some situations.

@schlichtanders
Copy link

a real pity... one of my packages relies heavily on recursion, where it is very natural to write with indirect recursion.

Are there any plans to improve type-inference for such indirect recursion for upcoming Julia version? Like in one year it is roughly planned to have this in julia? (I know, I could try fixing it myself, but it is already quite tough to find time for maintaining and improving my own Julia packages)

@vtjnash
Copy link
Member

vtjnash commented Dec 15, 2022

we may fix it, but the timeline of getting someone to implement and test it is unclear

vtjnash added a commit that referenced this issue Jul 27, 2023
vtjnash added a commit that referenced this issue Aug 10, 2023
vtjnash added a commit that referenced this issue Aug 14, 2023
Fix #45759
Fix #46557
Fix #31485

Depends on #50694 due to a failing broadcast test without it (related to
#50695)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants