-
-
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: enable constant propagation for invoke
d calls, fixes #41024
#41383
Conversation
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG |
It turns out that this PR allows # master
julia> code_llvm(()) do
sin(sum(sincos(10)))
end
; @ REPL[16]:2 within `#23`
define double @"julia_#23_867"() #0 {
top:
%0 = call double @j_sin_869(double 0xBFF62125BF7FDD38) #0
ret double %0
}
# this PR
julia> code_llvm(()) do
sin(sum(sincos(10)))
end
; @ REPL[12]:2 within `#19`
define double @"julia_#19_911"() #0 {
top:
ret double 0xBFEF701C34D1CD3B
} And so it wins some scalar benchmarks completely, e.g.:
|
Hmm, with certain compile time cost of course,... # master
julia> @time using Plots
3.564731 seconds (7.13 M allocations: 496.071 MiB, 6.93% gc time)
julia> @time plot(rand(10,3))
2.662668 seconds (3.51 M allocations: 219.314 MiB, 5.52% gc time, 55.19% compilation time)
# this PR
ulia> @time using Plots
3.810949 seconds (7.33 M allocations: 513.128 MiB, 6.46% gc time)
julia> @time plot(rand(10,3))
2.867559 seconds (3.52 M allocations: 219.522 MiB, 5.04% gc time, 55.26% compilation time) |
Okay 8028bc1 improves inference itself a bit and should fix the latency regression: # master
julia> @time using Plots; @time plot(rand(10,3))
4.126655 seconds (7.13 M allocations: 496.086 MiB, 7.48% gc time)
2.908765 seconds (3.51 M allocations: 219.314 MiB, 4.60% gc time, 53.35% compilation time)
# after 8028bc1
julia> @time using Plots; @time plot(rand(10,3))
3.889846 seconds (7.01 M allocations: 489.443 MiB, 7.06% gc time)
2.918426 seconds (3.50 M allocations: 218.861 MiB, 4.42% gc time, 54.79% compilation time) |
edge !== nothing && add_backedge!(edge::MethodInstance, sv) | ||
return CallMeta(rt, InvokeCallInfo(MethodMatch(ti, env, method, argtype <: method.sig))) | ||
match = MethodMatch(ti, env, method, argtype <: method.sig) | ||
# try constant propagation with early take-in of the heuristics -- since constuctions below seem to be a bit costly |
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 know if this manual inlining is worth it compared to the cost of duplication. Inference is already very expensive and the invoke
path is not particularly hot.
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.
Ah, I guess you did benchmark this. LGTM then.
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.
Yeah, it actually reduced memory allocation (and compile time):
# without manual inlining
julia> @time using Plots
3.810949 seconds (7.33 M allocations: 513.128 MiB, 6.46% gc time)
# with manual inlining
julia> @time using Plots
4.252111 seconds (7.01 M allocations: 489.437 MiB, 6.27% gc time)
That suggests, some logic within abstract_call_method_with_const_args
is costly and we may want to refactor it so to gain the same improvements for abstract_call_gf_by_type
too, but that would be a separate PR.
Especially useful for defining mixins with typed interface fields, e.g. ```julia abstract type AbstractInterface end # mixin, which expects common field `x::Int` function Base.getproperty(x::AbstractInterface, sym::Symbol) if sym === :x return getfield(x, sym)::Int # inferred field else return getfield(x, sym) # fallback end end abstract type AbstractInterfaceExtended <: AbstractInterface end # extended mixin, which expects additional common field `y::Rational{Int}` function Base.getproperty(x::AbstractInterfaceExtended, sym::Symbol) if sym === :y return getfield(x, sym)::Rational{Int} end return Base.@invoke getproperty(x::AbstractInterface, sym::Symbol) end ``` As a bonus, inliner is able to use `InferenceResult` as a fast inlining pass for constant-prop'ed `invoke`s
8636a53
to
3fbb6f0
Compare
Going to merge this tmrw if there is no objection. |
(#41383) * inference: enable constant propagation for `invoke`d calls, fixes #41024 Especially useful for defining mixins with typed interface fields, e.g. ```julia abstract type AbstractInterface end # mixin, which expects common field `x::Int` function Base.getproperty(x::AbstractInterface, sym::Symbol) if sym === :x return getfield(x, sym)::Int # inferred field else return getfield(x, sym) # fallback end end abstract type AbstractInterfaceExtended <: AbstractInterface end # extended mixin, which expects additional common field `y::Rational{Int}` function Base.getproperty(x::AbstractInterfaceExtended, sym::Symbol) if sym === :y return getfield(x, sym)::Rational{Int} end return Base.@invoke getproperty(x::AbstractInterface, sym::Symbol) end ``` As a bonus, inliner is able to use `InferenceResult` as a fast inlining pass for constant-prop'ed `invoke`s * improve compile-time latency * Update base/compiler/abstractinterpretation.jl * Update base/compiler/abstractinterpretation.jl (cherry picked from commit bc6da93)
Nice. Does this fix #29285? |
No, this PR only "fixes" the one specific case when we want LICM. |
Looks like this caused #41417, and CI even failed with that error on this PR... |
Thanks, I simply overlooked that error. |
Now special functions including `sincos` are fully constant folded, and these test break due to it. The use such functions that won't never be constant-folded instead.
…iaLang#41024 (JuliaLang#41383) * inference: enable constant propagation for `invoke`d calls, fixes JuliaLang#41024 Especially useful for defining mixins with typed interface fields, e.g. ```julia abstract type AbstractInterface end # mixin, which expects common field `x::Int` function Base.getproperty(x::AbstractInterface, sym::Symbol) if sym === :x return getfield(x, sym)::Int # inferred field else return getfield(x, sym) # fallback end end abstract type AbstractInterfaceExtended <: AbstractInterface end # extended mixin, which expects additional common field `y::Rational{Int}` function Base.getproperty(x::AbstractInterfaceExtended, sym::Symbol) if sym === :y return getfield(x, sym)::Rational{Int} end return Base.@invoke getproperty(x::AbstractInterface, sym::Symbol) end ``` As a bonus, inliner is able to use `InferenceResult` as a fast inlining pass for constant-prop'ed `invoke`s * improve compile-time latency * Update base/compiler/abstractinterpretation.jl * Update base/compiler/abstractinterpretation.jl
Can we revert this PR and then re-merge it with a fix? |
thank you, nicely done . "We have the technology. We have the capability to build.. that.. Better than.. before. Better, stronger, faster." |
Especially useful for defining mixins with typed interface fields, e.g.
As a bonus, inliner is able to use
InferenceResult
as a fast inliningpass for constant-prop'ed
invoke
sinvoke
d calls #41024runbenchmarks(ALL, vs=":master")