-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Conversation
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. |
This crashes during bootstrap, because in inlining we have some special IncrementalCompact usage sharing state: julia/base/compiler/ssair/inlining.jl Lines 422 to 423 in 9fd4087
julia/base/compiler/ssair/ir.jl Lines 630 to 643 in 9fd4087
@Keno what's the exact intended behavior of 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 |
89729b9
to
20aa918
Compare
20aa918
to
ae81c88
Compare
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 @nanosoldier |
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. |
That's more of an improvement than I expected... |
Let's see if this reproduces if we test against the exact merge-base: @nanosoldier |
Your benchmark job has completed - no performance regressions were detected. A full report can be found here. |
Seems to be better or equal and fixes the issue so SGTM |
Fixes #46945