Skip to content

Adjust backedge insertion #32

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 13, 2025
Merged

Adjust backedge insertion #32

merged 1 commit into from
May 13, 2025

Conversation

Keno
Copy link
Member

@Keno Keno commented May 12, 2025

Together with JuliaLang/julia#58390 fixes #31.

@serenity4 we may need a better test for the invalidation of the factory method. It was broken, but the existing tests didn't catch that.

@Keno Keno requested a review from serenity4 May 12, 2025 21:18
serenity4
serenity4 reviewed May 12, 2025
View reviewed changes
src/transform/common.jl
nothing, debuginfo, edges)
ccall(:jl_method_instance_add_backedge, Cvoid, (Any, Any, Any), mi, nothing, daef_ci)
Compiler.store_backedges(daef_ci, edges)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

store_backedges will ignore a few CodeInstances that we generate, that's why I used jl_method_instance_add_backedge. We might want to remove this check in Compiler instead, but I don't know what the implications are/what were the motivations to have it in the first place.

Suggested change
Compiler.store_backedges(daef_ci, edges)
ccall(:jl_method_instance_add_backedge, Cvoid, (Any, Any, Any), old_ci, nothing, daef_ci)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this should be picked up by tests)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we're using any CodeInstances that don't have an associated method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall exactly what we had, but this check did trigger for some. That's most likely what broke the tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that check in base needs to use a get_ci_mi(ci).def instead so that our ABI override code instances work properly, but I didn't think we put those into edges

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The check is for callers not edges, which are the ABI override code instances.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Yes, submit a PR to base to put in the get_ci_mi then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR: JuliaLang/julia#58394
I confirmed that this change allows store_backedges to work correctly with our CodeInstances, instead of needing to call jl_method_instance_add_backedge manually.

@serenity4
Copy link
Member

Thanks for the fix. I managed to reproduce the issue once, but this seems either non-deterministic or evaluation order-dependent, and now I can't reproduce it.

@serenity4
Copy link
Member

serenity4 commented May 12, 2025 •
edited
Loading

It does depend on evaluation order. If I execute the invalidation tests first, then I can't observe the bug, no matter how I invalidate/reexecute code.

The test suite broke on latest nightly, and I get the invalid world age error only after triggering the error and then doing refresh(). If these observations are correct, I don't think we can test that reliably.

Stacktrace for nightly failure 
ERROR: LoadError: FieldError: type String has no field `extended_rt`; String has no fields at all.
Stacktrace:
  [1] getproperty(x::String, f::Symbol)
    @ Base ./Base_compiler.jl:57
  [2] abstract_eval_invoke_inst(interp::DAECompiler.StructuralRefiner, inst::Compiler.Instruction, irsv::Compiler.IRInterpretationState)
    @ DAECompiler ~/.julia/dev/DAECompiler/src/analysis/refiner.jl:54
  [3] reprocess_instruction!(interp::DAECompiler.StructuralRefiner, inst::Compiler.Instruction, idx::Int64, bb::Int64, irsv::Compiler.IRInterpretationState)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:168
  [4] (::Compiler.var"#217#218"{BitSet, DAECompiler.StructuralRefiner, Compiler.IRInterpretationState, Compiler.var"#check_ret!#216"{Vector{Int64}}, BitSet, Compiler.TwoPhaseDefUseMap, Compiler.IRCode})(inst::Compiler.Instruction, lstmt::Int64, bb::Int64)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:364
  [5] scan!(callback::Compiler.var"#217#218"{BitSet, DAECompiler.StructuralRefiner, Compiler.IRInterpretationState, Compiler.var"#check_ret!#216"{Vector{Int64}}, BitSet, Compiler.TwoPhaseDefUseMap, Compiler.IRCode}, scanner::Compiler.BBScanner, forwards_only::Bool)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:286
  [6] ir_abstract_constant_propagation(interp::DAECompiler.StructuralRefiner, irsv::Compiler.IRInterpretationState; externally_refined::BitSet)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:332
  [7] _structural_analysis!(ci::Core.CodeInstance, world::UInt64)
    @ DAECompiler ~/.julia/dev/DAECompiler/src/analysis/structural.jl:160
  [8] structural_analysis!(ci::Core.CodeInstance, world::UInt64)
    @ DAECompiler ~/.julia/dev/DAECompiler/src/analysis/structural.jl:26
  [9] abstract_eval_invoke_inst(interp::DAECompiler.StructuralRefiner, inst::Compiler.Instruction, irsv::Compiler.IRInterpretationState)
    @ DAECompiler ~/.julia/dev/DAECompiler/src/analysis/refiner.jl:52
 [10] reprocess_instruction!(interp::DAECompiler.StructuralRefiner, inst::Compiler.Instruction, idx::Int64, bb::Int64, irsv::Compiler.IRInterpretationState)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:168
 [11] (::Compiler.var"#217#218"{BitSet, DAECompiler.StructuralRefiner, Compiler.IRInterpretationState, Compiler.var"#check_ret!#216"{Vector{Int64}}, BitSet, Compiler.TwoPhaseDefUseMap, Compiler.IRCode})(inst::Compiler.Instruction, lstmt::Int64, bb::Int64)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:364
 [12] scan!(callback::Compiler.var"#217#218"{BitSet, DAECompiler.StructuralRefiner, Compiler.IRInterpretationState, Compiler.var"#check_ret!#216"{Vector{Int64}}, BitSet, Compiler.TwoPhaseDefUseMap, Compiler.IRCode}, scanner::Compiler.BBScanner, forwards_only::Bool)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:286
 [13] ir_abstract_constant_propagation(interp::DAECompiler.StructuralRefiner, irsv::Compiler.IRInterpretationState; externally_refined::BitSet)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:332
 [14] _structural_analysis!(ci::Core.CodeInstance, world::UInt64)
    @ DAECompiler ~/.julia/dev/DAECompiler/src/analysis/structural.jl:160
 [15] structural_analysis!(ci::Core.CodeInstance, world::UInt64)
    @ DAECompiler ~/.julia/dev/DAECompiler/src/analysis/structural.jl:26
 [16] abstract_eval_invoke_inst(interp::DAECompiler.StructuralRefiner, inst::Compiler.Instruction, irsv::Compiler.IRInterpretationState)
    @ DAECompiler ~/.julia/dev/DAECompiler/src/analysis/refiner.jl:52
 [17] reprocess_instruction!(interp::DAECompiler.StructuralRefiner, inst::Compiler.Instruction, idx::Int64, bb::Int64, irsv::Compiler.IRInterpretationState)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:168
 [18] (::Compiler.var"#217#218"{BitSet, DAECompiler.StructuralRefiner, Compiler.IRInterpretationState, Compiler.var"#check_ret!#216"{Vector{Int64}}, BitSet, Compiler.TwoPhaseDefUseMap, Compiler.IRCode})(inst::Compiler.Instruction, lstmt::Int64, bb::Int64)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:364
 [19] scan!(callback::Compiler.var"#217#218"{BitSet, DAECompiler.StructuralRefiner, Compiler.IRInterpretationState, Compiler.var"#check_ret!#216"{Vector{Int64}}, BitSet, Compiler.TwoPhaseDefUseMap, Compiler.IRCode}, scanner::Compiler.BBScanner, forwards_only::Bool)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:286
 [20] ir_abstract_constant_propagation(interp::DAECompiler.StructuralRefiner, irsv::Compiler.IRInterpretationState; externally_refined::BitSet)
    @ Compiler ~/.julia/packages/Compiler/X1WCU/src/ssair/irinterp.jl:332
 [21] _structural_analysis!(ci::Core.CodeInstance, world::UInt64)
    @ DAECompiler ~/.julia/dev/DAECompiler/src/analysis/structural.jl:160
 [22] structural_analysis!(ci::Core.CodeInstance, world::UInt64)
    @ DAECompiler ~/.julia/dev/DAECompiler/src/analysis/structural.jl:26
 [23] factory_gen(world::UInt64, source::Method, _gen::Any, settings::Type, fT::Any)
    @ DAECompiler ~/.julia/dev/DAECompiler/src/interface.jl:32
 [24] get_concrete_problem(prob::DAECompiler.DAECProblem{typeof(Main.ipo.sin2!), Tuple{Pair{Int64, Float64}, Pair{Int64, Float64}}, Nothing, Tuple{Float64, Float64}, @Kwargs{}}, isadaptive::Bool; kwargs::@Kwargs{u0::Nothing, p::Missing})
    @ DAECompiler ~/.julia/dev/DAECompiler/src/problem_interface.jl:31
 [25] get_concrete_problem
    @ ~/.julia/dev/DAECompiler/src/problem_interface.jl:30 [inlined]
 [26] solve_up(prob::DAECompiler.DAECProblem{typeof(Main.ipo.sin2!), Tuple{Pair{Int64, Float64}, Pair{Int64, Float64}}, Nothing, Tuple{Float64, Float64}, @Kwargs{}}, sensealg::Nothing, u0::Nothing, p::Missing, args::Sundials.IDA{:Dense, Nothing, Nothing}; kwargs::@Kwargs{})
    @ DiffEqBase ~/.julia/packages/DiffEqBase/3HGYN/src/solve.jl:1193
 [27] solve_up
    @ ~/.julia/packages/DiffEqBase/3HGYN/src/solve.jl:1177 [inlined]
 [28] solve(prob::DAECompiler.DAECProblem{typeof(Main.ipo.sin2!), Tuple{Pair{Int64, Float64}, Pair{Int64, Float64}}, Nothing, Tuple{Float64, Float64}, @Kwargs{}}, args::Sundials.IDA{:Dense, Nothing, Nothing}; sensealg::Nothing, u0::Nothing, p::Nothing, wrap::Val{true}, kwargs::@Kwargs{})
    @ DiffEqBase ~/.julia/packages/DiffEqBase/3HGYN/src/solve.jl:1089
 [29] solve(prob::DAECompiler.DAECProblem{typeof(Main.ipo.sin2!), Tuple{Pair{Int64, Float64}, Pair{Int64, Float64}}, Nothing, Tuple{Float64, Float64}, @Kwargs{}}, args::Sundials.IDA{:Dense, Nothing, Nothing})
    @ DiffEqBase ~/.julia/packages/DiffEqBase/3HGYN/src/solve.jl:1079
 [30] top-level scope
    @ ~/.julia/dev/DAECompiler/test/ipo.jl:44

@Keno
Copy link
Member Author

Keno commented May 12, 2025

It does depend on evaluation order. If I execute the invalidation tests first, then I can't observe the bug, no matter how I invalidate/reexecute code.

There needs to be an old CodeInstance for the same method in the cache. It should be reproducible by running a test from ipo.jl, then doing refresh() and then rerunning the same test without re-defining the method. I'm not seeing the failure you posted, but I can take a look later or tomorrow.

@serenity4
Copy link
Member

I can't reproduce with the non-erroring tests, there seems to be a correlation with the new failure mode and the invalid world range error path. Method redefinition doesn't seem to have any effect for me.

@serenity4
Copy link
Member

Ah, I still had https://github.com/CedarEDA/Compiler.jl in my manifest instead of https://github.com/JuliaLang/BaseCompiler.jl. The "new error on nightly" was a false alert, it's fixed by using BaseCompiler.

@serenity4
Copy link
Member

I can't reproduce it at all anymore. I'll be happy to add a test for it if you manage to get a reproducer.

@Keno Keno merged commit 75eb76f into main May 13, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serenity4 serenity4 serenity4 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Structural analysis can see invalid world age after refresh
2 participants
@Keno @serenity4