Skip to content

Commit

Permalink
add additional required copies for opt_call, but disable it
Browse files Browse the repository at this point in the history
Reverts #308, as this still seems insufficient to make evals>1 work correctly.
  • Loading branch information
vtjnash committed Jul 8, 2024
1 parent 9e78db8 commit 1bc6851
Showing 1 changed file with 12 additions and 4 deletions.
16 changes: 12 additions & 4 deletions src/inference/InferenceBenchmarks.jl
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ using Core:
using .CC:
AbstractInterpreter, InferenceParams, InferenceResult, InferenceState,
OptimizationParams, OptimizationState, WorldRange, WorldView,
specialize_method, unwrap_unionall, rewrap_unionall
specialize_method, unwrap_unionall, rewrap_unionall, copy
@static if VERSION v"1.11.0-DEV.1498"
import .CC: get_inference_world
else
Expand Down Expand Up @@ -167,9 +167,14 @@ end
function opt_call(@nospecialize(f), @nospecialize(types = Base.default_tt(f));
interp::InferenceBenchmarker = InferenceBenchmarker())
frame = inf_call(f, types; interp, run_optimizer = false)
evals = 0
return function ()
# `optimize` may modify these objects, so stash the pre-optimization states
src, stmt_info = copy(frame.src), copy(frame.stmt_info)
@assert (evals += 1) <= 1
## `optimize` may modify these objects, so need to stash the pre-optimization states, if we want to allow multiple evals
#src, stmt_info, slottypes, ssavalue_uses = copy(frame.src), copy(frame.stmt_info), copy(frame.slottypes), copy(frame.ssavalue_uses)
#cfg = @static hasfield(InferenceState, :cfg) ? copy(frame.cfg) : nothing
#unreachable = @static hasfield(InferenceState, :unreachable) ? copy(frame.unreachable) : nothing
#bb_vartables = @static hasfield(InferenceState, :bb_vartables) ? copy(frame.bb_vartables) : nothing
@static if !hasfield(Core.Compiler.InliningState, :params)
opt = OptimizationState(frame, interp)
CC.optimize(interp, opt, frame.result)
Expand All @@ -178,7 +183,10 @@ function opt_call(@nospecialize(f), @nospecialize(types = Base.default_tt(f));
opt = OptimizationState(frame, params, interp)
CC.optimize(interp, opt, params, frame.result)
end
frame.src, frame.stmt_info = src, stmt_info
#frame.src, frame.stmt_info, frame.slottypes, frame.ssavalue_uses = src, stmt_info, slottypes, ssavalue_uses
#cfg === nothing || (frame.cfg = cfg)
#unreachable === nothing || (frame.unreachable = unreachable)
#bb_vartables === nothing || (frame.bb_vartables = bb_vartables)
end
end

Expand Down

3 comments on commit 1bc6851

@vtjnash
Copy link
Member Author

@vtjnash vtjnash commented on 1bc6851 Jul 8, 2024

Choose a reason for hiding this comment

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

@aviatesk I meant to make this a PR for your comment, but then accidentally pushed it directly. I suppose it doesn't matter, since this isn't the nanosoldier branch anyways. I also added branch protection for master, so accidental pushes won't happen again to it.

@aviatesk
Copy link
Member

Choose a reason for hiding this comment

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

Even if we revert the PR and comment it out, the problem will not be solved (since evals needs to be used as a keyword in the @benchmarkable macro). Should we revert this commit as well for now?

@vtjnash
Copy link
Member Author

@vtjnash vtjnash commented on 1bc6851 Jul 9, 2024

Choose a reason for hiding this comment

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

I was hoping to fix it (since right now it crashes in inlining with the fixes, and in the verifier without them), but couldn't figure out the inlining bug and realized I didn't really need this to fix nanosoldier for now

Please sign in to comment.