inference: enable constant propagation for `invoke`d calls, fixes #41024 by aviatesk · Pull Request #41383 · JuliaLang/julia

aviatesk · 2021-06-26T19:56:50Z

Especially useful for defining mixins with typed interface fields, e.g.

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 invokes

closes inference: enable constant propagation for invoked calls #41024
@nanosoldier runbenchmarks(ALL, vs=":master")

nanosoldier · 2021-06-27T02:10:53Z

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG

aviatesk · 2021-06-27T04:17:56Z

It turns out that this PR allows @horner to be constant-folded:

# 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.:

ID	time ratio	memory ratio
`["scalar", "sincos", ("argument reduction (easy) abs(x) < 2.0^20π/4", "negative argument", "Float64")]`	0.00 (5%) ✅	1.00 (1%)
`["scalar", "sincos", ("argument reduction (easy) abs(x) < 2.0^20π/4", "positive argument", "Float64")]`	0.00 (5%) ✅	1.00 (1%)
`["scalar", "sincos", ("argument reduction (easy) abs(x) < 2π/4", "negative argument", "Float32")]`	0.00 (5%) ✅	1.00 (1%)
`["scalar", "sincos", ("argument reduction (easy) abs(x) < 2π/4", "negative argument", "Float64")]`	0.00 (5%) ✅	1.00 (1%)

aviatesk · 2021-06-27T04:38:43Z

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)

aviatesk · 2021-06-27T06:04:44Z

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)

Keno · 2021-06-27T14:45:47Z

base/compiler/abstractinterpretation.jl

    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


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.

Ah, I guess you did benchmark this. LGTM then.

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.

base/compiler/abstractinterpretation.jl

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

aviatesk · 2021-06-29T16:07:48Z

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)

simonbyrne · 2021-06-30T16:40:40Z

Nice. Does this fix #29285?

aviatesk · 2021-06-30T17:11:46Z

No, this PR only "fixes" the one specific case when we want LICM.
We should fix it more generally, i.e. even if a target component can't be constant-folded, we want to hoist it out of loop as far as it proven to be loop-invariant.

fredrikekre · 2021-07-01T08:28:43Z

Looks like this caused #41417, and CI even failed with that error on this PR...

aviatesk · 2021-07-01T08:36:46Z

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

DilumAluthge · 2021-07-05T20:12:38Z

Can we revert this PR and then re-merge it with a fix?

…ixes #41024 (#41383)" This reverts commit bc6da93.

JeffreySarnoff · 2021-07-05T21:29:43Z

thank you, nicely done .

"We have the technology. We have the capability to build.. that.. Better than.. before. Better, stronger, faster."
— The Six Million Dollar Man, Opening Narration [heavily redacted]

aviatesk requested review from JeffBezanson, Keno and vtjnash June 26, 2021 19:57

Keno reviewed Jun 27, 2021

View reviewed changes

aviatesk commented Jun 27, 2021

View reviewed changes

base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved

base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved

aviatesk added backport 1.7 compiler:inference Type inference labels Jun 27, 2021

aviatesk closed this Jun 28, 2021

aviatesk reopened this Jun 28, 2021

aviatesk and others added 4 commits June 29, 2021 15:33

improve compile-time latency

369f4a0

Update base/compiler/abstractinterpretation.jl

b3f9e75

Update base/compiler/abstractinterpretation.jl

3fbb6f0

aviatesk force-pushed the avi/constinvoke branch from 8636a53 to 3fbb6f0 Compare June 29, 2021 09:09

aviatesk merged commit bc6da93 into master Jun 30, 2021

aviatesk deleted the avi/constinvoke branch June 30, 2021 01:34

aviatesk mentioned this pull request Jun 30, 2021

release-1.7: Backports for 1.7-RC1/1.7-beta3 #41382

Merged

51 tasks

fredrikekre mentioned this pull request Jul 1, 2021

Error in testset mpfr #41417

Closed

DilumAluthge added a commit that referenced this pull request Jul 5, 2021

Revert "inference: enable constant propagation for invoked calls, f…

31a15f9

…ixes #41024 (#41383)" This reverts commit bc6da93.

DilumAluthge mentioned this pull request Jul 5, 2021

Revert "inference: enable constant propagation for invoked calls, fixes #41024" #41472

Closed

KristofferC removed the backport 1.7 label Jul 7, 2021

Uh oh!

Conversation

aviatesk commented Jun 26, 2021

Uh oh!

nanosoldier commented Jun 27, 2021

Uh oh!

aviatesk commented Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviatesk commented Jun 27, 2021

Uh oh!

aviatesk commented Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Keno Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

Keno Jun 27, 2021

Choose a reason for hiding this comment

Uh oh!

aviatesk Jun 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aviatesk commented Jun 29, 2021

Uh oh!

simonbyrne commented Jun 30, 2021

Uh oh!

aviatesk commented Jun 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredrikekre commented Jul 1, 2021

Uh oh!

aviatesk commented Jul 1, 2021

Uh oh!

DilumAluthge commented Jul 5, 2021

Uh oh!

JeffreySarnoff commented Jul 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

aviatesk commented Jun 27, 2021 •

edited

Loading

aviatesk commented Jun 27, 2021 •

edited

Loading

aviatesk Jun 27, 2021 •

edited

Loading

aviatesk commented Jun 30, 2021 •

edited

Loading