Skip to content
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

IncrementalCompact: fix stateful behavior when using multiple iterators. #46952

Merged
merged 4 commits into from
Oct 6, 2022

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Sep 28, 2022

Fixes #46945

@maleadt maleadt added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Sep 28, 2022
@Keno
Copy link
Member

Keno commented Sep 28, 2022

Seems good to me. I think there's some argument that maybe it should restart at the beginning and iterate the already-compacted stmts first, but I'm ok with the stateful version also.

@maleadt
Copy link
Member Author

maleadt commented Sep 29, 2022

This crashes during bootstrap, because in inlining we have some special IncrementalCompact usage sharing state:

inline_compact = IncrementalCompact(compact, spec.ir, compact.result_idx)
for ((_, idx′), stmt′) in inline_compact

# For inlining
function IncrementalCompact(parent::IncrementalCompact, code::IRCode, result_offset)
perm = my_sortperm(Int[code.new_nodes.info[i].pos for i in 1:length(code.new_nodes)])
new_len = length(code.stmts) + length(code.new_nodes)
ssa_rename = Any[SSAValue(i) for i = 1:new_len]
bb_rename = Vector{Int}()
pending_nodes = NewNodeStream()
pending_perm = Int[]
return new(code, parent.result,
parent.result_bbs, ssa_rename, bb_rename, bb_rename, parent.used_ssas,
parent.late_fixup, perm, 1,
parent.new_new_nodes, parent.new_new_used_ssas, pending_nodes, pending_perm,
1, result_offset, parent.active_result_bb, false, false, false)
end

@Keno what's the exact intended behavior of IncrementalCompact(::IncrementalCompact)?


EDIT: apparently that's used by inlining, to splice IR into another function:

julia> parent(i) = i + 42
julia> ir = only(Base.code_ircode(parent, (Int,)))[1]
1 1%1 = Base.add_int(_2, 42)::Int64
  └──      return %1

# partial compaction (advancing to the point we'll inline at)
julia> compact = Core.Compiler.IncrementalCompact(ir)
julia> state = Core.Compiler.iterate(compact)
1 1%1 = Base.add_int(_2, 42)::Int64
─────────────────────────────────────────────────────────────────
  └──      return %1

# code to inline
julia> child() = ccall(:jl_breakpoint, Nothing, (Any,), nothing)
julia> inline = only(Base.code_ircode(child, ()))[1]
1 1%1 = $(Expr(:foreigncall, :(:jl_breakpoint), Nothing, svec(Any), 0, :(:ccall), :(Main.nothing)))::Core.Const(nothing)
  └──      return %1

# iterator to splice both together
julia> inline_compact = Core.Compiler.IncrementalCompact(compact, inline, compact.result_idx)
1 1%1 = Base.add_int(_2, 42)::Int64
─────────────────────────────────────────────────────────────────
1 1%1 = $(Expr(:foreigncall, :(:jl_breakpoint), Nothing, svec(Any), 0, :(:ccall), :(Main.nothing)))::Core.Const(nothing)
  └──      return %1

# iterating it will inline the code
julia> Core.Compiler.iterate(inline_compact)
1 1 ─     Base.add_int(_2, 42)::Int64
  └──     $(Expr(:foreigncall, :(:jl_breakpoint), Nothing, svec(Any), 0, :(:ccall), :(Main.nothing)))::Core.Const(nothing)
─────────────────────────────────────────────────────────────────
  !!! ─      return %1

# not complete the original iterator
julia> while state !== nothing
           state = Core.Compiler.iterate(compact, state[2])
       end
julia> ir = Core.Compiler.complete(compact)
1 1%1 = Base.add_int(_2, 42)::Int64$(Expr(:foreigncall, :(:jl_breakpoint), Nothing, svec(Any), 0, :(:ccall), :(Main.nothing)))::Core.Const(nothing)
  └──      return %1

@maleadt maleadt marked this pull request as draft September 29, 2022 11:32
@maleadt maleadt force-pushed the tb/ircompact_statefulness branch from 89729b9 to 20aa918 Compare October 5, 2022 07:48
@maleadt maleadt force-pushed the tb/ircompact_statefulness branch from 20aa918 to ae81c88 Compare October 5, 2022 10:50
@maleadt
Copy link
Member Author

maleadt commented Oct 5, 2022

Found the issue: the iterator state was tracking the active bb, not the active result bb, so I added a field to IncrementalCompact to keep track of that, and use it during initialization of the iterator.

Now, with active_bb being part of IncrementalCompact, we can remove the actual iterator state completely. Since that code is pretty hot, let's see how it affects performance:

@nanosoldier runbenchmarks("inference", vs=":master")

@maleadt maleadt marked this pull request as ready for review October 5, 2022 10:54
@nanosoldier
Copy link
Collaborator

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

@maleadt
Copy link
Member Author

maleadt commented Oct 5, 2022

That's more of an improvement than I expected...

@maleadt
Copy link
Member Author

maleadt commented Oct 5, 2022

Let's see if this reproduces if we test against the exact merge-base:

@nanosoldier runbenchmarks("inference", vs="@f056dbc78172eb1aec1a3b41a4f9b15d3683306e")

@nanosoldier
Copy link
Collaborator

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

@gbaraldi
Copy link
Member

gbaraldi commented Oct 5, 2022

Seems to be better or equal and fixes the issue so SGTM

@maleadt maleadt merged commit a3c7d7f into master Oct 6, 2022
@maleadt maleadt deleted the tb/ircompact_statefulness branch October 6, 2022 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IncrementalCompact re-iteration is broken
6 participants