optimizer: fix #42754, inline union-split const-prop'ed sources by aviatesk · Pull Request #42766 · JuliaLang/julia

aviatesk · 2021-10-22T20:02:12Z

This commit complements #39754 and #39305: implements a logic to use
constant-prop'ed results for inlining at union-split callsite.
Currently it works only for cases when constant-prop' succeeded for all
(union-split) signatures.

example

julia> mutable struct X
           # NOTE in order to confuse `fieldtype_tfunc`, we need to have at least two fields with different types
           a::Union{Nothing, Int}
           b::Symbol
       end;

julia> code_typed((X, Union{Nothing,Int})) do x, a
           # this `setproperty` call would be union-split and constant-prop will happen for
           # each signature: inlining would fail if we don't use constant-prop'ed source
           # since the approximated inlining cost of `convert(fieldtype(X, sym), a)` would
           # end up very high if we don't propagate `sym::Const(:a)`
           x.a = a
           x
       end |> only |> first

before this commit

CodeInfo(
1 ─ %1 = Base.setproperty!::typeof(setproperty!)
│   %2 = (isa)(a, Nothing)::Bool
└──      goto #3 if not %2
2 ─ %4 = π (a, Nothing)
│        invoke %1(_2::X, :a::Symbol, %4::Nothing)::Any
└──      goto #6
3 ─ %7 = (isa)(a, Int64)::Bool
└──      goto #5 if not %7
4 ─ %9 = π (a, Int64)
│        invoke %1(_2::X, :a::Symbol, %9::Int64)::Any
└──      goto #6
5 ─      Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{}
└──      unreachable
6 ┄      return x
)

after this commit

CodeInfo(
1 ─ %1 = (isa)(a, Nothing)::Bool
└──      goto #3 if not %1
2 ─      Base.setfield!(x, :a, nothing)::Nothing
└──      goto #6
3 ─ %5 = (isa)(a, Int64)::Bool
└──      goto #5 if not %5
4 ─ %7 = π (a, Int64)
│        Base.setfield!(x, :a, %7)::Int64
└──      goto #6
5 ─      Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{}
└──      unreachable
6 ┄      return x
)

This PR also comes with 774ca28, which
improves code qualities of inlining code, most notably eliminates
excessive specializations around Pair{Any, Any} construction/zipping.

aviatesk · 2021-10-23T05:54:46Z

IIRC this change will require a manual cherry pick for backporting.

@nanosoldier runbenchmarks("foldl" || "sort" || "union", vs = ":master")

nanosoldier · 2021-10-23T06:51:40Z

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

KristofferC · 2021-10-23T10:24:28Z

Would be good to get it backported to prevent the performance regression.

aviatesk · 2021-10-23T10:37:13Z

ok I'll take it once we merge this PR.

aviatesk · 2021-10-24T16:47:23Z

I also added general improvements around the inlining code as well.
Especially, 774ca28 eliminates excessive specializations around Pair{Any, Any} constructions and zipping Vector{Pair{Any,Any}}.
As a result, the # of precompiled statements is reduced to 1469/1504 from 1483/1518.

test/compiler/inline.jl

vtjnash

SGTM

This commit includes several code quality improvements in inlining code: - eliminate excessive specializations around: * `item::Pair{Any, Any}` constructions * iterations on `Vector{Pair{Any, Any}}` - replace `Pair{Any, Any}` with new, more explicit data type `InliningCase` - remove dead code

aviatesk · 2021-10-26T06:57:33Z

I found we need to make sure we don't inline !isdispatchtuple(sig) case when union-splitting.
MRE:

# validate inlining processing
import Base: @constprop

@constprop :none @inline isType(@nospecialize(t)) = throw("invalid inlining processing detected")
@constprop :none @noinline isType(i::Integer) = (println(IOBuffer(), "prevent inlining"); false)
let
    invoke(xs) = isType(xs[1])
    @test invoke(Any[10]) === false
end

@constprop :aggressive @inline isType(c, @nospecialize(t)) = c && throw("invalid inlining processing detected")
@constprop :aggressive @noinline isType(c, i::Integer) = c && (println(IOBuffer(), "prevent inlining"); false)
let
    invoke(xs) = isType(true, xs[1])
    @test invoke(Any[10]) === false
end

aviatesk · 2021-10-26T09:42:49Z

@nanosoldier runbenchmarks("foldl" || "sort" || "union", vs = ":master")

nanosoldier · 2021-10-26T10:40:50Z

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

This commit complements #39754 and #39305: implements a logic to use constant-prop'ed results for inlining at union-split callsite. Currently it works only for cases when constant-prop' succeeded for all (union-split) signatures. > example ```julia julia> mutable struct X # NOTE in order to confuse `fieldtype_tfunc`, we need to have at least two fields with different types a::Union{Nothing, Int} b::Symbol end; julia> code_typed((X, Union{Nothing,Int})) do x, a # this `setproperty` call would be union-split and constant-prop will happen for # each signature: inlining would fail if we don't use constant-prop'ed source # since the approximated inlining cost of `convert(fieldtype(X, sym), a)` would # end up very high if we don't propagate `sym::Const(:a)` x.a = a x end |> only |> first ``` > before this commit ```julia CodeInfo( 1 ─ %1 = Base.setproperty!::typeof(setproperty!) │ %2 = (isa)(a, Nothing)::Bool └── goto #3 if not %2 2 ─ %4 = π (a, Nothing) │ invoke %1(_2::X, 🅰️:Symbol, %4::Nothing)::Any └── goto #6 3 ─ %7 = (isa)(a, Int64)::Bool └── goto #5 if not %7 4 ─ %9 = π (a, Int64) │ invoke %1(_2::X, 🅰️:Symbol, %9::Int64)::Any └── goto #6 5 ─ Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{} └── unreachable 6 ┄ return x ) ``` > after this commit ```julia CodeInfo( 1 ─ %1 = (isa)(a, Nothing)::Bool └── goto #3 if not %1 2 ─ Base.setfield!(x, :a, nothing)::Nothing └── goto #6 3 ─ %5 = (isa)(a, Int64)::Bool └── goto #5 if not %5 4 ─ %7 = π (a, Int64) │ Base.setfield!(x, :a, %7)::Int64 └── goto #6 5 ─ Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{} └── unreachable 6 ┄ return x ) ```

aviatesk · 2021-10-26T12:31:54Z

@nanosoldier runbenchmarks("foldl" || "sort" || "union", vs = ":master")

nanosoldier · 2021-10-26T13:30:35Z

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

aviatesk · 2021-10-26T13:45:05Z

This PR should be solid on my responsibility, and I'm going to merge (and then backport to v1.7) once I confirmed green CIs and no suspicious logs.

backport #42766 into `backports-release-1.7`

commit c054dbc Author: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Date: Fri Oct 29 01:31:55 2021 +0900 optimizer: eliminate allocations (JuliaLang#42833) commit 6a9737d Author: Jeff Bezanson <jeff.bezanson@gmail.com> Date: Thu Oct 28 12:23:53 2021 -0400 fix JuliaLang#42659, move `jl_coverage_visit_line` to runtime library (JuliaLang#42810) commit c762f10 Author: Marc Ittel <35898736+MarcMush@users.noreply.github.com> Date: Thu Oct 28 12:19:13 2021 +0200 change `julia` to `julia-repl` in docstrings (JuliaLang#42824) Co-authored-by: Michael Abbott <32575566+mcabbott@users.noreply.github.com> commit 9f52ec0 Author: Dilum Aluthge <dilum@aluthge.com> Date: Thu Oct 28 05:30:11 2021 -0400 CI (Buildkite): Update all rootfs images to the latest versions (JuliaLang#42802) * CI (Buildkite): Update all rootfs images to the latest versions * Re-sign all of the signed pipelines commit 404e584 Author: DilumAluthgeBot <43731525+DilumAluthgeBot@users.noreply.github.com> Date: Wed Oct 27 21:11:04 2021 -0400 🤖 Bump the Statistics stdlib from 74897fe to 5256d57 (JuliaLang#42826) Co-authored-by: Dilum Aluthge <dilum@aluthge.com> commit c74814e Author: Jeff Bezanson <jeff.bezanson@gmail.com> Date: Wed Oct 27 16:34:46 2021 -0400 reset `RandomDevice` file from `__init__` (JuliaLang#42537) This prevents us from seeing an invalid `IOStream` object from a saved system image, and also ensures the files are opened once for all threads. commit 05ed348 Author: Jeff Bezanson <jeff.bezanson@gmail.com> Date: Wed Oct 27 15:24:17 2021 -0400 only visit nonfunction_mt once when traversing method tables (JuliaLang#42821) commit d71b77d Author: DilumAluthgeBot <43731525+DilumAluthgeBot@users.noreply.github.com> Date: Tue Oct 26 20:39:08 2021 -0400 🤖 Bump the Downloads stdlib from 5f1509d to dbb0625 (JuliaLang#42811) Co-authored-by: Dilum Aluthge <dilum@aluthge.com> commit b4fddc1 Author: DilumAluthgeBot <43731525+DilumAluthgeBot@users.noreply.github.com> Date: Tue Oct 26 14:46:20 2021 -0400 🤖 Bump the Pkg stdlib from bc32103f to 26918395 (JuliaLang#42806) Co-authored-by: Dilum Aluthge <dilum@aluthge.com> commit 6a386de Author: Dilum Aluthge <dilum@aluthge.com> Date: Tue Oct 26 12:15:51 2021 -0400 CI (Buildkite): make sure to hit ignore any unencrypted repo keys, regardless of where they are located in the repository (JuliaLang#42803) commit 021a6b5 Author: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Date: Wed Oct 27 01:08:33 2021 +0900 optimizer: clean up inlining test code (JuliaLang#42804) commit 16eb196 Merge: 21ebabf 1510eaa Author: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Date: Tue Oct 26 23:25:41 2021 +0900 Merge pull request JuliaLang#42766 from JuliaLang/avi/42754 optimizer: fix JuliaLang#42754, inline union-split const-prop'ed sources commit 21ebabf Author: Kristoffer Carlsson <kcarlsson89@gmail.com> Date: Tue Oct 26 16:11:32 2021 +0200 simplify code loading test now that TOML files are parsed with a real TOML parser (JuliaLang#42328) commit 1510eaa Author: Shuhei Kadowaki <aviatesk@gmail.com> Date: Mon Oct 25 01:35:12 2021 +0900 optimizer: fix JuliaLang#42754, inline union-split const-prop'ed sources This commit complements JuliaLang#39754 and JuliaLang#39305: implements a logic to use constant-prop'ed results for inlining at union-split callsite. Currently it works only for cases when constant-prop' succeeded for all (union-split) signatures. > example ```julia julia> mutable struct X # NOTE in order to confuse `fieldtype_tfunc`, we need to have at least two fields with different types a::Union{Nothing, Int} b::Symbol end; julia> code_typed((X, Union{Nothing,Int})) do x, a # this `setproperty` call would be union-split and constant-prop will happen for # each signature: inlining would fail if we don't use constant-prop'ed source # since the approximated inlining cost of `convert(fieldtype(X, sym), a)` would # end up very high if we don't propagate `sym::Const(:a)` x.a = a x end |> only |> first ``` > before this commit ```julia CodeInfo( 1 ─ %1 = Base.setproperty!::typeof(setproperty!) │ %2 = (isa)(a, Nothing)::Bool └── goto #3 if not %2 2 ─ %4 = π (a, Nothing) │ invoke %1(_2::X, 🅰️:Symbol, %4::Nothing)::Any └── goto #6 3 ─ %7 = (isa)(a, Int64)::Bool └── goto #5 if not %7 4 ─ %9 = π (a, Int64) │ invoke %1(_2::X, 🅰️:Symbol, %9::Int64)::Any └── goto #6 5 ─ Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{} └── unreachable 6 ┄ return x ) ``` > after this commit ```julia CodeInfo( 1 ─ %1 = (isa)(a, Nothing)::Bool └── goto #3 if not %1 2 ─ Base.setfield!(x, :a, nothing)::Nothing └── goto #6 3 ─ %5 = (isa)(a, Int64)::Bool └── goto #5 if not %5 4 ─ %7 = π (a, Int64) │ Base.setfield!(x, :a, %7)::Int64 └── goto #6 5 ─ Core.throw(ErrorException("fatal error in type inference (type bound)"))::Union{} └── unreachable 6 ┄ return x ) ``` commit 4c3ae20 Author: Chris Foster <chris42f@gmail.com> Date: Tue Oct 26 21:48:32 2021 +1000 Make Base.ifelse a generic function (JuliaLang#37343) Allow user code to directly extend `Base.ifelse` rather than needing a special package for it. commit 2e388e3 Author: Shuhei Kadowaki <aviatesk@gmail.com> Date: Mon Oct 25 01:30:09 2021 +0900 optimizer: eliminate excessive specialization in inlining code This commit includes several code quality improvements in inlining code: - eliminate excessive specializations around: * `item::Pair{Any, Any}` constructions * iterations on `Vector{Pair{Any, Any}}` - replace `Pair{Any, Any}` with new, more explicit data type `InliningCase` - remove dead code

#42841)

… by JuliaLang#42766 (JuliaLang#42841)

aviatesk requested review from Keno and vtjnash October 22, 2021 20:02

aviatesk added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Oct 22, 2021

KristofferC added the backport 1.7 label Oct 22, 2021

aviatesk force-pushed the avi/42754 branch from dd57605 to 13d9503 Compare October 24, 2021 16:36

aviatesk force-pushed the avi/42754 branch 2 times, most recently from 217f877 to fe18ecf Compare October 25, 2021 06:37

vtjnash reviewed Oct 25, 2021

View reviewed changes

test/compiler/inline.jl Outdated Show resolved Hide resolved

vtjnash approved these changes Oct 25, 2021

View reviewed changes

aviatesk added the DO NOT MERGE Do not merge this PR! label Oct 26, 2021

aviatesk force-pushed the avi/42754 branch from fe18ecf to 3cb8b37 Compare October 26, 2021 09:03

aviatesk removed the DO NOT MERGE Do not merge this PR! label Oct 26, 2021

aviatesk force-pushed the avi/42754 branch from 3cb8b37 to 1510eaa Compare October 26, 2021 12:28

vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Oct 26, 2021

aviatesk mentioned this pull request Oct 26, 2021

backport #42766 into backports-release-1.7 #42805

Merged

aviatesk merged commit 16eb196 into master Oct 26, 2021

aviatesk deleted the avi/42754 branch October 26, 2021 14:25

aviatesk mentioned this pull request Oct 27, 2021

release-1.7: Backports for 1.7.0/1.7.0-rc3 #42765

Merged

66 tasks

aviatesk added a commit that referenced this pull request Oct 27, 2021

Merge pull request #42805 from JuliaLang/avi/backport-42766

f662b0a

backport #42766 into `backports-release-1.7`

DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Oct 28, 2021

aviatesk added a commit that referenced this pull request Oct 29, 2021

optimizer: fix #42840, the performance regression introduced by #42766

7b792ca

aviatesk added a commit that referenced this pull request Oct 29, 2021

optimizer: fix #42840, the performance regression introduced by #42766

5abfc25

aviatesk added a commit that referenced this pull request Oct 29, 2021

optimizer: fix #42840, the performance regression introduced by #42766 (

b285b1d

#42841)

aviatesk added a commit that referenced this pull request Oct 29, 2021

optimizer: fix #42840, the performance regression introduced by #42766 (

83bd616

#42841)

aviatesk added a commit that referenced this pull request Oct 30, 2021

optimizer: fix #42840, the performance regression introduced by #42766 (

382129f

#42841)

KristofferC removed the backport 1.7 label Nov 15, 2021

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022

optimizer: fix JuliaLang#42840, the performance regression introduced…

9ba584d

… by JuliaLang#42766 (JuliaLang#42841)

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022

optimizer: fix JuliaLang#42840, the performance regression introduced…

e5af603

… by JuliaLang#42766 (JuliaLang#42841)

Uh oh!

Conversation

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

Uh oh!

Uh oh!

aviatesk commented Oct 23, 2021

Uh oh!

nanosoldier commented Oct 23, 2021

Uh oh!

KristofferC commented Oct 23, 2021

Uh oh!

aviatesk commented Oct 23, 2021

Uh oh!

aviatesk commented Oct 24, 2021

Uh oh!

Uh oh!

vtjnash left a comment

Choose a reason for hiding this comment

Uh oh!

aviatesk commented Oct 26, 2021

Uh oh!

aviatesk commented Oct 26, 2021

Uh oh!

nanosoldier commented Oct 26, 2021

Uh oh!

aviatesk commented Oct 26, 2021

Uh oh!

nanosoldier commented Oct 26, 2021

Uh oh!

aviatesk commented Oct 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aviatesk commented Oct 22, 2021 •

edited

Loading