Skip to content

Commit

Permalink
concrete-eval: Use concrete eval information even if the result is to…
Browse files Browse the repository at this point in the history
…o large

When we originally implemented concrete-eval, we decided not to create `Const`
lattice elements for constants that are too large, on the fear that these
would end up in the IR and blowing up the size. Now that we have some
experience with this, I think that decision was incorrect for several reasons:

1. We've already performed the concrete evaluation (and allocated the big
   object), so we're just throwing away precision here that we could
   have otherwise used (Although if the call result is unused, we probably
   shouldn't do concrete eval at all - see #46774).
2. There's a number of other places in the compiler where we read large
   values into `Const`. Unless we add these kinds of check there too,
   we need to have appropriate guards in the optimizer and the cache
   anyway, to prevent the IR size blowup.
3. It turns out that throwing away this precision actually causes
   significant performance problems for code that is just over the line.
   Consider for example a lookup of a small struct inside a large,
   multi-level constant structure. The final result might be quite
   tiny, but if we refuse to propagate the intermediate levels,
   we might end up doing an (expensive) full-constprop when propagating
   the constant information could have given us a much cheaper
   concrete evaluation.

This commit simply removes that check. If we see any regressions
as a result of this, we should see if there are additional guards
needed in the optimizer.
  • Loading branch information
Keno authored and aviatesk committed Oct 23, 2022
1 parent 54e6899 commit 209d943
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 17 deletions.
8 changes: 1 addition & 7 deletions base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -841,13 +841,7 @@ function concrete_eval_call(interp::AbstractInterpreter,
# The evaluation threw. By :consistent-cy, we're guaranteed this would have happened at runtime
return ConstCallResults(Union{}, ConcreteResult(edge, result.effects), result.effects, edge)
end
if is_inlineable_constant(value) || call_result_unused(si)
# If the constant is not inlineable, still do the const-prop, since the
# code that led to the creation of the Const may be inlineable in the same
# circumstance and may be optimizable.
return ConstCallResults(Const(value), ConcreteResult(edge, EFFECTS_TOTAL, value), EFFECTS_TOTAL, edge)
end
return false
return ConstCallResults(Const(value), ConcreteResult(edge, EFFECTS_TOTAL, value), EFFECTS_TOTAL, edge)
else # eligible for semi-concrete evaluation
return true
end
Expand Down
21 changes: 11 additions & 10 deletions base/compiler/ssair/inlining.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1169,7 +1169,7 @@ function handle_invoke_call!(todo::Vector{Pair{Int,Any}},
end
result = info.result
invokesig = sig.argtypes
if isa(result, ConcreteResult)
if isa(result, ConcreteResult) && may_inline_concrete_result(result)
item = concrete_result_item(result, state; invokesig)
else
argtypes = invoke_rewrite(sig.argtypes)
Expand Down Expand Up @@ -1296,7 +1296,11 @@ function handle_any_const_result!(cases::Vector{InliningCase},
@nospecialize(info::CallInfo), flag::UInt8, state::InliningState;
allow_abstract::Bool, allow_typevars::Bool)
if isa(result, ConcreteResult)
return handle_concrete_result!(cases, result, state)
if may_inline_concrete_result(result)
return handle_concrete_result!(cases, result, state)
else
result = nothing
end
end
if isa(result, SemiConcreteResult)
result = inlining_policy(state.interp, result, info, flag, result.mi, argtypes)
Expand Down Expand Up @@ -1483,15 +1487,12 @@ function handle_concrete_result!(cases::Vector{InliningCase}, result::ConcreteRe
return true
end

may_inline_concrete_result(result::ConcreteResult) =
isdefined(result, :result) && is_inlineable_constant(result.result)

function concrete_result_item(result::ConcreteResult, state::InliningState;
invokesig::Union{Nothing,Vector{Any}}=nothing)
if !isdefined(result, :result) || !is_inlineable_constant(result.result)
et = InliningEdgeTracker(state.et, invokesig)
case = compileable_specialization(result.mi, result.effects, et;
compilesig_invokes=state.params.compilesig_invokes)
@assert case !== nothing "concrete evaluation should never happen for uncompileable callsite"
return case
end
@assert may_inline_concrete_result(result)
@assert result.effects === EFFECTS_TOTAL
return ConstantCase(quoted(result.result))
end
Expand Down Expand Up @@ -1524,7 +1525,7 @@ function handle_opaque_closure_call!(todo::Vector{Pair{Int,Any}},
mi = result.result.linfo
validate_sparams(mi.sparam_vals) || return nothing
item = resolve_todo(mi, result.result, sig.argtypes, info, flag, state)
elseif isa(result, ConcreteResult)
elseif isa(result, ConcreteResult) && may_inline_concrete_result(result)
item = concrete_result_item(result, state)
else
item = analyze_method!(info.match, sig.argtypes, info, flag, state; allow_typevars=false)
Expand Down

0 comments on commit 209d943

Please sign in to comment.