inference: track reaching defs for slots#55601
Conversation
0879cc3 to
5ffbc97
Compare
4829608 to
e60a5ee
Compare
aviatesk
left a comment
There was a problem hiding this comment.
Thanks, this looks much better than my PR in general.
Still I would like to add the test case from it, i.e.
# JuliaLang/julia#55548: invalidate stale slot wrapper types in `ssavaluetypes`
_issue55548_proj1(a, b) = a
function issue55548(a)
a = Base.inferencebarrier(a)::Union{Int64,Float64}
if _issue55548_proj1(isa(a, Int64), (a = Base.inferencebarrier(1.0)::Union{Int64,Float64}; true))
return a
end
return 2
end
@test Float64 <: Base.infer_return_type(issue55548, (Int,))|
@nanosoldier |
|
Thanks for the test case and review @aviatesk fwiw, there's also a precision regression that I encountered w/ this PR: function foo()
value = @noinline rand((true, false, 1))
is_bool = value isa Bool
if inferencebarrier(@noinline rand(Bool))
value = 1.0
is_bool = false
end
# at this merge point, only the "false half" of the Conditional should be invalidated
return is_bool ? value : false
end
@assert only(Base.return_types(foo, ())) === Bool # fails w/ PRbut I think this can be mostly fixed by widening the |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
bump? are we able to land this and backport to v1.11/v1.10? |
e614050 to
eca1168
Compare
|
As discussed with @topolarity, I will take over this PR and finish it so we can have a fix for the underlying issue. It seems like most of the core functionality works well at the moment, so I believe it's mostly a matter of finishing up the design and related refactors, and address performance regressions. |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
@serenity4 are you still planning to mark this ready for review in time for v1.12 backports? |
|
Thanks for the ping. Yes, I plan to work again on it soon-ish, not sure it will be ready for a backport for 1.12.0 but at least 1.12.1 would be nice. That is, unless we have a strong motivation to get it in for 1.12.0, in which case I can divert more resources to it. |
Add a new `StateRefinement` struct that guarantees object identity is preserved by the update (i.e. `x (before) === x (after)`), in contrast to `StateUpdate` which represents a new assignment. This allows us to remove the `conditional` flag from `StateUpdate` and the `ignore_conditional` parameter from `invalidate_slotwrapper`. Also change `conditional_change` and `condition_object_change` to use a `Symbol` parameter (`:then` or `:else`) instead of a `Bool` for clarity. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Cédric Belmant <cedric.belmant@juliahub.com> Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add `vtypes::Union{VarTable,Nothing}` parameter to abstract call functions
to make the current VarTable available during call analysis. This enables
future improvements to track SSA definition sites for slot values.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Cédric Belmant <cedric.belmant@juliahub.com>
Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add `ssadef` field to `VarState`, `Conditional`, and `MustAlias` to track the "reaching definition" for slot variables. This enables tracking object identity across control flow, which is useful for later refinements. The ssadef represents: - 0: value comes from an argument - negative: refers to a "virtual ϕ-block" preceding the given index When a slot has the same ssadef at two different points of execution, the slot contents are guaranteed to share identity (x₀ === x₁). This commit updates: - VarState to include ssadef field - Conditional and MustAlias to include ssadef field - smerge/schanged/stupdate! to compute reaching definitions using the path-convergence criterion for SSA construction - All callsites to construct these types with the new field - widenreturn to only convert to Inter* types when ssadef == 0 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Cédric Belmant <cedric.belmant@juliahub.com> Co-Authored-By: Jameson Nash <vtjnash+github@gmail.com> Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
With ssadef tracking, we no longer need to explicitly invalidate Conditional/MustAlias lattice elements when a slot is reassigned. The ssadef field in these types records which "version" of the slot they refer to, and conditional_valid() detects when a Conditional is stale (i.e., when the slot has been reassigned since the Conditional was created). This removes: - invalidate_slotwrapper(): no longer needed - widenwrappedslotwrapper(): was only used by invalidate_slotwrapper - The invalidation loop in stoverwrite1!() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@nanosoldier |
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
|
I just had Claude finish the TODO items, since they sounded like very mechanical things. Seems to have done fine, so this seems ready to merge now, pending review and author approval. |
|
Has the regression in #55601 (comment) been fixed? We should also run a quick evaluation to make sure this doesn't make inference too slow (it turns inference into an semi-inefficient re-implementation of ϕ-node insertion, so it could slow down some cases) |
No, I had a commit for it, but it was tricky enough that I wanted to make it a separate PR
It performed better in nanosoldier than our current implementation |
|
@nanosoldier |
|
The package evaluation job you requested has completed - possible new issues were detected. Report summary❗ Packages that crashed2 packages crashed only on the current version.
25 packages crashed on the previous version too. ✖ Packages that failed11 packages failed only on the current version.
2802 packages failed on the previous version too. ✔ Packages that passed tests3 packages passed tests only on the current version.
3924 packages passed tests on the previous version too. ~ Packages that at least loaded3537 packages successfully loaded on the previous version too. ➖ Packages that were skipped altogether1 packages were skipped only on the current version.
916 packages were skipped on the previous version too. |
DRAFT of how this should be implemented
|
I see the @topolarity Would you be able to push a manual backport to |
This PR implements the "Path-Convergence Criterion" for SSA / ϕ-nodes as part of type-inference.
The
VarState.ssadeffield corresponds to the "reaching definition" of the slot in SSA form, which allows us to conveniently reason about the identity of aslotacross multiple program points. If the reaching def is equal at two program points, then the slot contents are guaranteed to be egal (i.e.x₀ === x₁)Tasks:
StateRefinementchange to separate PRVarTableplumbing to separate PRConditionalinvalidation logicFixes #55548 (alternative to #55551), by having
Conditionalremember the reaching definition that it narrows.