Skip to content

Commit

Permalink
disable irinterp for interpreters with may_optimize(...)=false (#53580
Browse files Browse the repository at this point in the history
)

As discussed at
<b8a0a39#commitcomment-139076159>,
currently external abstract interpreter that configures `may_optimize`
to return `false` may end up with the internal error from irinterp since
it fundamentally required optimized IR but it currently assumes that all
sources from cached `CodeInstance`s are optimized.

This commit addresses the issue by incorporating a `may_optimize` check
in `concrete_eval_eligible`, which in turn automatically disables
irinterp for such interpreters. Although there were earlier discussions
suggesting the revival of `codeinfo.inferred::Bool`, this commit does
not need it, and I think this approach maintains the current state more
cleanly.

This should fix the error of `"inference"` benchmarks from
BaseBenchmarks.jl.
  • Loading branch information
aviatesk authored Mar 4, 2024
1 parent 0311aa4 commit 144f58b
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 1 deletion.
7 changes: 6 additions & 1 deletion base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -897,7 +897,12 @@ function concrete_eval_eligible(interp::AbstractInterpreter,
add_remark!(interp, sv, "[constprop] Concrete eval disabled for overlayed methods")
end
if !any_conditional(arginfo)
return :semi_concrete_eval
if may_optimize(interp)
return :semi_concrete_eval
else
# disable irinterp if optimization is disabled, since it requires optimized IR
add_remark!(interp, sv, "[constprop] Semi-concrete interpretation disabled for non-optimizing interpreter")
end
end
end
return :none
Expand Down
11 changes: 11 additions & 0 deletions test/compiler/AbstractInterpreter.jl
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,17 @@ const CC = Core.Compiler
include("irutils.jl")
include("newinterp.jl")

# interpreter that performs abstract interpretation only
# (semi-concrete interpretation should be disabled automatically)
@newinterp AbsIntOnlyInterp1
CC.may_optimize(::AbsIntOnlyInterp1) = false
@test Base.infer_return_type(Base.init_stdio, (Ptr{Cvoid},); interp=AbsIntOnlyInterp1()) >: IO

# it should work even if the interpreter discards inferred source entirely
@newinterp AbsIntOnlyInterp2
CC.may_optimize(::AbsIntOnlyInterp2) = false
CC.transform_result_for_cache(::AbsIntOnlyInterp2, ::Core.MethodInstance, ::CC.WorldRange, ::CC.InferenceResult) = nothing
@test Base.infer_return_type(Base.init_stdio, (Ptr{Cvoid},); interp=AbsIntOnlyInterp2()) >: IO

# OverlayMethodTable
# ==================
Expand Down

3 comments on commit 144f58b

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on 144f58b Mar 4, 2024

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on 144f58b Mar 5, 2024

Choose a reason for hiding this comment

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

Inference took another large hit again in performance and allocations. It might be this commit, but I didn’t think this commit was supposed to affect this test too

["inference", "allinference", "Base.init_stdio(::Ptr{Cvoid})"] 5.90 (5%) ❌ 5.69 (1%) ❌

But rest looked pretty good. Collections and push seemed much faster, though not clear what commit could have altered that

Please sign in to comment.