improve inferrabilities by telling the compiler relational invariants#40594
improve inferrabilities by telling the compiler relational invariants#40594JeffBezanson merged 1 commit intoJuliaLang:masterfrom
Conversation
simeonschaub
left a comment
There was a problem hiding this comment.
Could you perhaps add a short comment to each of these functions explaining that this is because we don't do relational analysis yet, just because the code does look a bit odd at first glance? Also, is there any way to test this by checking the inferred return types?
base/multidimensional.jl
Outdated
| @assert isa(f2, Tuple{}) | ||
| _isless(newret, f1, f2) |
There was a problem hiding this comment.
Is @assert the right tool for these kind of hints? We have been talking about eliminating them unless in debug mode, so perhaps something like
| @assert isa(f2, Tuple{}) | |
| _isless(newret, f1, f2) | |
| _isless(newret, f1, f2::Tuple{}) |
would be better?
There was a problem hiding this comment.
Thanks for your comment.
My original motivation to use @assert instead of typeassert was that otherwise JET will report false positive errors on the typeassertions for some complicated cases where inference is unsure about the type of one tuple but sure about that of the other, e.g.:
example false positive reports
under the definition of
_islessbelow@inline function _isless(ret, I1::NTuple{N,Int}, I2::NTuple{N,Int}) where N newret = ifelse(ret==0, icmp(I1[N], I2[N]), ret) f1, f2 = Base.front(I1), Base.front(I2) if isa(f1, Tuple{}) return _isless(newret, f1, f2::Tuple{}) # L133 else return _isless(newret, f1, f2::Tuple{Int,Vararg{Int}}) # L135 end end
julia> report_call((Int, Tuple{Vararg{Int}}, Tuple{Int,Int,Vararg{Int}})) do ret, t1, t2
Base.IteratorsMD._isless(ret, t1, t2)
end
═════ 2 possible errors found ═════
┌ @ none:2 Base.getproperty(Base.IteratorsMD, :_isless)(ret, t1, t2)
│┌ @ /Users/aviatesk/julia/julia/base/multidimensional.jl:133 Core.typeassert(f2, Core.apply_type(Base.IteratorsMD.Tuple))
││ invalid builtin function call: Core.typeassert(f2::Union{Tuple{Int64}, Tuple{Int64, Int64, Vararg{Int64}}}, Core.apply_type(Base.IteratorsMD.Tuple)::Type{Tuple{}})
│└────────────────────────────────────────────────────────────
│┌ @ /Users/aviatesk/julia/julia/base/multidimensional.jl:135 Base.IteratorsMD._isless(newret, f1, Core.typeassert(f2, Core.apply_type(Base.IteratorsMD.Tuple, Base.IteratorsMD.Int, Core.apply_type(Base.IteratorsMD.Vararg, Base.IteratorsMD.Int))))
││┌ @ /Users/aviatesk/julia/julia/base/multidimensional.jl:135 Core.typeassert(f2, Core.apply_type(Base.IteratorsMD.Tuple, Base.IteratorsMD.Int, Core.apply_type(Base.IteratorsMD.Vararg, Base.IteratorsMD.Int)))
│││ invalid builtin function call: Core.typeassert(f2::Tuple{}, Core.apply_type(Base.IteratorsMD.Tuple, Base.IteratorsMD.Int, Core.apply_type(Base.IteratorsMD.Vararg, Base.IteratorsMD.Int)::Core.TypeofVararg)::Type{Tuple{Int64, Vararg{Int64}}})
││└────────────────────────────────────────────────────────────and with @assert JET can forcibly exclude those cases (but yes, this is just because JET's bug report criteria is somewhat looser on manually thrown exceptions than type failures...)
Anyway, I totally agree that @assert is very unnatural to be here, and there is no strong reason to make changes just for the sake of JET.
In the latest commit (bb0bff9) I focused on eliminating dynamic dispatches by adding type annotations (and also exclude some possibilities for JET to report false positives).
Hopefully in this direction we can agree with taking these changes in ?
There was a problem hiding this comment.
Yes, your version looks reasonable to me.
ca13b0d to
bb0bff9
Compare
Our compiler doesn't understand these relations automatically yet.
simeonschaub
left a comment
There was a problem hiding this comment.
LGTM.
On a related note: Did you ever think about running some of JET's analysis on base as part of CI? Making it a required check is probably difficult, since JET would always have to be updated with internal base changes right away, but perhaps it could run as a scheduled Job, similar to PkgEval and BaseBenchmarks? I am a bit worried that now that we are getting quite careful when it comes to inference and invalidations in base, it will also be really easy to accidentally introduce regressions in that area. @timholy might have some thoughts on this as well.
…JuliaLang#40594) Our compiler doesn't understand these relations automatically yet.
…JuliaLang#40594) Our compiler doesn't understand these relations automatically yet.
…JuliaLang#40594) Our compiler doesn't understand these relations automatically yet.
…JuliaLang#40594) Our compiler doesn't understand these relations automatically yet.
...by removing unnecessary type checks/assertions. This basically restores the function bodies to what they looked like prior to JuliaLang#40594 and only keeps the changed signatures (with the additional changes to circumvent JuliaLang#39088.
...by removing unnecessary type checks/assertions. This basically restores the function bodies to what they looked like prior to JuliaLang#40594 and only keeps the changed signatures (with the additional changes to circumvent JuliaLang#39088.
Our compiler doesn't understand these relations automatically yet.