-
-
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
inference: forward Conditional
inter-procedurally
#42529
Conversation
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 great! I had a read through though I don't know the surrounding code well enough to review properly.
4d598c7
to
06f196a
Compare
Alright, I think this PR is ready to go. @vtjnash can I ask your review on this ? |
3ed9124
to
018e46d
Compare
018e46d
to
ac5d042
Compare
ac5d042
to
e9aaea2
Compare
@vtjnash I'd like to merge this PR tomorrow on my responsibility, but I'd appreciate if you could review it if you have a time. |
base/compiler/inferenceresult.jl
Outdated
# union-split should never find a more precise information about `fargs[slotid]` than `Conditional`, | ||
# otherwise there should have been a bug around `Conditional` construction or maintainance |
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 don't believe this is true, since Conditional is actually tri-stated. Consider:
f(::Int) = 1
f(::Float64) = 2
f(::String) = 3
x::Any
if x isa Union{Int, Float64}
cond = x isa Int
end
f(cond, x) # x was Any before examining `cond::Conditional(:x, Int, Float64)`, but now the signature for `String` is excluded
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.
Right, thanks for your review.
I added 28f50c6 so that we just propagate Bottom
in such a case (where we accidentally prove the method match is impossible).
Now this code snippet doesn't throw:
Base.@constprop :aggressive f(cnd, ::Int) = cnd
Base.@constprop :aggressive f(cnd, ::Float64) = !cnd
Base.@constprop :aggressive f(cnd, ::String) = cnd
function g()
x = v::Any
if x isa Union{Int, Float64}
cnd = x isa Int # ::Conditional(:x, Int, Float64)
end
f(cnd, x)
end
@code_typed optimize=false g()
The PR #38905 only "back-propagates" conditional constraint (from callee to caller), but currently we don't "forward" it (caller to callee), and so inter-procedural constraint propagation won't happen for e.g.: ```julia ifelselike(cnd, x, y) = cnd ? x : y @test Base.return_types((Any,Int,)) do x, y ifelselike(isa(x, Int), x, y) end |> only == Int ``` This commit complements #38905 and enables further inter-procedural conditional constraint propagation by forwarding `Conditional` to callees when it imposes a constraint on any other argument, during constant propagation.
- remove `const_prop_rettype_heuristic` since it handles rare cases, where const-prop' doens't seem to be worthwhile, e.g. it won't be so useful to try to propagate `Const(Tuple{DataType,DataType})` for `Const(convert)(::Const(Tuple{DataType,DataType}), ::Tuple{DataType,DataType} -> Tuple{DataType,DataType}` - rename `is_allconst` to `is_all_overridden` - also minor refactors and improvements added
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
701a2a1
to
28f50c6
Compare
28f50c6
to
c641d85
Compare
JET reported: ```julia julia> report_package(JET) ... [toplevel-info] analyzing from top-level definitions ... 671/671 ═════ 12 possible errors found ═════ ... ┌ @ avi-/JET/src/JET.jl:990 #self#(text, "top-level") │┌ @ avi-/JET/src/JET.jl:990 JET.#report_text#124(JET.JETAnalyzer, JET.nothing, Base.pairs(Core.NamedTuple()), #self#, text, filename) ││┌ @ avi-/JET/src/JET.jl:993 res = JET.virtual_process(text, filename, analyzer′, config) │││┌ @ avi-/JET/src/toplevel/virtualprocess.jl:301 res = JET._virtual_process!(x, filename, analyzer, config, context, JET.VirtualProcessResult(actual2virtual, context)) ││││┌ @ avi-/JET/src/toplevel/virtualprocess.jl:432 res = JET._virtual_process!(toplevelex, filename, analyzer, config, context, res) │││││┌ @ avi-/JET/src/toplevel/virtualprocess.jl:637 JET.analyze_toplevel!(analyzer, src) ││││││┌ @ avi-/JET/src/JET.jl:1142 JET.#analyze_toplevel!#133(true, #self#, analyzer, src) │││││││┌ @ avi-/JET/src/JET.jl:1160 JET.analyze_frame!(analyzer, frame) ││││││││┌ @ avi-/JET/src/JET.jl:747 JET.typeinf(analyzer, frame) │││││││││┌ @ compiler/abstractinterpretation.jl:1243 Core.Compiler.lastindex(fargs) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.lastindex), Nothing})): Core.Compiler.lastindex(fargs::Union{Nothing, Vector{Any}}) │││││││││└─────────────────────────────────────────── │││││││││┌ @ compiler/abstractinterpretation.jl:1243 fargs′ = Core.Compiler.getindex(fargs, Core.Compiler.:(3, Core.Compiler.lastindex(fargs))) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.getindex), Nothing, Core.Compiler.UnitRange{Int64}})): fargs′ = Core.Compiler.getindex(fargs::Union{Nothing, Vector{Any}}, Core.Compiler.:(3, Core.Compiler.lastindex(fargs::Union{Nothing, Vector{Any}})::Int64)::Core.Compiler.UnitRange{Int64}) │││││││││└─────────────────────────────────────────── │││││││││┌ @ compiler/abstractinterpretation.jl:1244 Core.Compiler.getindex(fargs, 1) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.getindex), Nothing, Int64})): Core.Compiler.getindex(fargs::Union{Nothing, Vector{Any}}, 1) │││││││││└─────────────────────────────────────────── ... ```
JET reported: ```julia julia> report_package(JET) ... [toplevel-info] analyzing from top-level definitions ... 671/671 ═════ 12 possible errors found ═════ ... ┌ @ avi-/JET/src/JET.jl:990 #self#(text, "top-level") │┌ @ avi-/JET/src/JET.jl:990 JET.#report_text#124(JET.JETAnalyzer, JET.nothing, Base.pairs(Core.NamedTuple()), #self#, text, filename) ││┌ @ avi-/JET/src/JET.jl:993 res = JET.virtual_process(text, filename, analyzer′, config) │││┌ @ avi-/JET/src/toplevel/virtualprocess.jl:301 res = JET._virtual_process!(x, filename, analyzer, config, context, JET.VirtualProcessResult(actual2virtual, context)) ││││┌ @ avi-/JET/src/toplevel/virtualprocess.jl:432 res = JET._virtual_process!(toplevelex, filename, analyzer, config, context, res) │││││┌ @ avi-/JET/src/toplevel/virtualprocess.jl:637 JET.analyze_toplevel!(analyzer, src) ││││││┌ @ avi-/JET/src/JET.jl:1142 JET.#analyze_toplevel!#133(true, #self#, analyzer, src) │││││││┌ @ avi-/JET/src/JET.jl:1160 JET.analyze_frame!(analyzer, frame) ││││││││┌ @ avi-/JET/src/JET.jl:747 JET.typeinf(analyzer, frame) │││││││││┌ @ compiler/abstractinterpretation.jl:1243 Core.Compiler.lastindex(fargs) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.lastindex), Nothing})): Core.Compiler.lastindex(fargs::Union{Nothing, Vector{Any}}) │││││││││└─────────────────────────────────────────── │││││││││┌ @ compiler/abstractinterpretation.jl:1243 fargs′ = Core.Compiler.getindex(fargs, Core.Compiler.:(3, Core.Compiler.lastindex(fargs))) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.getindex), Nothing, Core.Compiler.UnitRange{Int64}})): fargs′ = Core.Compiler.getindex(fargs::Union{Nothing, Vector{Any}}, Core.Compiler.:(3, Core.Compiler.lastindex(fargs::Union{Nothing, Vector{Any}})::Int64)::Core.Compiler.UnitRange{Int64}) │││││││││└─────────────────────────────────────────── │││││││││┌ @ compiler/abstractinterpretation.jl:1244 Core.Compiler.getindex(fargs, 1) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.getindex), Nothing, Int64})): Core.Compiler.getindex(fargs::Union{Nothing, Vector{Any}}, 1) │││││││││└─────────────────────────────────────────── ... ```
…#42746) JET reported: ```julia julia> report_package(JET) ... [toplevel-info] analyzing from top-level definitions ... 671/671 ═════ 12 possible errors found ═════ ... │││││││││┌ @ compiler/abstractinterpretation.jl:1243 Core.Compiler.lastindex(fargs) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.lastindex), Nothing})): Core.Compiler.lastindex(fargs::Union{Nothing, Vector{Any}}) │││││││││└─────────────────────────────────────────── │││││││││┌ @ compiler/abstractinterpretation.jl:1243 fargs′ = Core.Compiler.getindex(fargs, Core.Compiler.:(3, Core.Compiler.lastindex(fargs))) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.getindex), Nothing, Core.Compiler.UnitRange{Int64}})): fargs′ = Core.Compiler.getindex(fargs::Union{Nothing, Vector{Any}}, Core.Compiler.:(3, Core.Compiler.lastindex(fargs::Union{Nothing, Vector{Any}})::Int64)::Core.Compiler.UnitRange{Int64}) │││││││││└─────────────────────────────────────────── │││││││││┌ @ compiler/abstractinterpretation.jl:1244 Core.Compiler.getindex(fargs, 1) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.getindex), Nothing, Int64})): Core.Compiler.getindex(fargs::Union{Nothing, Vector{Any}}, 1) │││││││││└─────────────────────────────────────────── ... ```
The PR JuliaLang#38905 only "back-propagates" conditional constraint (from callee to caller), but currently we don't "forward" it (caller to callee), and so inter-procedural constraint propagation won't happen for e.g.: ```julia ifelselike(cnd, x, y) = cnd ? x : y @test Base.return_types((Any,Int,)) do x, y ifelselike(isa(x, Int), x, y) end |> only == Int ``` This commit complements JuliaLang#38905 and enables further inter-procedural conditional constraint propagation by forwarding `Conditional` to callees when it imposes a constraint on any other argument, during constant propagation. We also improve constant-prop' heuristics in these ways: - remove `const_prop_rettype_heuristic` since it handles rare cases, where const-prop' doens't seem to be worthwhile, e.g. it won't be so useful to try to propagate `Const(Tuple{DataType,DataType})` for `Const(convert)(::Const(Tuple{DataType,DataType}), ::Tuple{DataType,DataType} -> Tuple{DataType,DataType}` - rename `is_allconst` to `is_all_overridden` - also minor refactors and improvements added
…t_invoke` (JuliaLang#42746) JET reported: ```julia julia> report_package(JET) ... [toplevel-info] analyzing from top-level definitions ... 671/671 ═════ 12 possible errors found ═════ ... │││││││││┌ @ compiler/abstractinterpretation.jl:1243 Core.Compiler.lastindex(fargs) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.lastindex), Nothing})): Core.Compiler.lastindex(fargs::Union{Nothing, Vector{Any}}) │││││││││└─────────────────────────────────────────── │││││││││┌ @ compiler/abstractinterpretation.jl:1243 fargs′ = Core.Compiler.getindex(fargs, Core.Compiler.:(3, Core.Compiler.lastindex(fargs))) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.getindex), Nothing, Core.Compiler.UnitRange{Int64}})): fargs′ = Core.Compiler.getindex(fargs::Union{Nothing, Vector{Any}}, Core.Compiler.:(3, Core.Compiler.lastindex(fargs::Union{Nothing, Vector{Any}})::Int64)::Core.Compiler.UnitRange{Int64}) │││││││││└─────────────────────────────────────────── │││││││││┌ @ compiler/abstractinterpretation.jl:1244 Core.Compiler.getindex(fargs, 1) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.getindex), Nothing, Int64})): Core.Compiler.getindex(fargs::Union{Nothing, Vector{Any}}, 1) │││││││││└─────────────────────────────────────────── ... ```
The PR JuliaLang#38905 only "back-propagates" conditional constraint (from callee to caller), but currently we don't "forward" it (caller to callee), and so inter-procedural constraint propagation won't happen for e.g.: ```julia ifelselike(cnd, x, y) = cnd ? x : y @test Base.return_types((Any,Int,)) do x, y ifelselike(isa(x, Int), x, y) end |> only == Int ``` This commit complements JuliaLang#38905 and enables further inter-procedural conditional constraint propagation by forwarding `Conditional` to callees when it imposes a constraint on any other argument, during constant propagation. We also improve constant-prop' heuristics in these ways: - remove `const_prop_rettype_heuristic` since it handles rare cases, where const-prop' doens't seem to be worthwhile, e.g. it won't be so useful to try to propagate `Const(Tuple{DataType,DataType})` for `Const(convert)(::Const(Tuple{DataType,DataType}), ::Tuple{DataType,DataType} -> Tuple{DataType,DataType}` - rename `is_allconst` to `is_all_overridden` - also minor refactors and improvements added
…t_invoke` (JuliaLang#42746) JET reported: ```julia julia> report_package(JET) ... [toplevel-info] analyzing from top-level definitions ... 671/671 ═════ 12 possible errors found ═════ ... │││││││││┌ @ compiler/abstractinterpretation.jl:1243 Core.Compiler.lastindex(fargs) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.lastindex), Nothing})): Core.Compiler.lastindex(fargs::Union{Nothing, Vector{Any}}) │││││││││└─────────────────────────────────────────── │││││││││┌ @ compiler/abstractinterpretation.jl:1243 fargs′ = Core.Compiler.getindex(fargs, Core.Compiler.:(3, Core.Compiler.lastindex(fargs))) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.getindex), Nothing, Core.Compiler.UnitRange{Int64}})): fargs′ = Core.Compiler.getindex(fargs::Union{Nothing, Vector{Any}}, Core.Compiler.:(3, Core.Compiler.lastindex(fargs::Union{Nothing, Vector{Any}})::Int64)::Core.Compiler.UnitRange{Int64}) │││││││││└─────────────────────────────────────────── │││││││││┌ @ compiler/abstractinterpretation.jl:1244 Core.Compiler.getindex(fargs, 1) ││││││││││ for 1 of union split cases, no matching method found for call signatures (Tuple{typeof(Core.Compiler.getindex), Nothing, Int64})): Core.Compiler.getindex(fargs::Union{Nothing, Vector{Any}}, 1) │││││││││└─────────────────────────────────────────── ... ```
The PR #38905 only "back-propagates" conditional constraint
(from callee to caller), but currently we don't "forward" it
(caller to callee), and so inter-procedural constraint propagation
won't happen for e.g.:
This commit complements #38905 and enables further inter-procedural
conditional constraint propagation by forwarding
Conditional
tocallees when it imposes a constraint on any other argument,
during constant propagation.