From 8daaf5c328ab3721c992965a8c81d47006172397 Mon Sep 17 00:00:00 2001 From: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com> Date: Tue, 16 Jan 2024 10:15:22 +0900 Subject: [PATCH] inference: always bail out const-prop' with non-const result limited (#52836) Investigating into #52763, I've found that `AbstractInterpreters` which enables the `aggressive_constprop` option, such as `REPLInterpreter`, might perform const-prop' even if the result of a non-const call is `LimitedAccuracy`. This can lead to an unintended infinite loop with a custom aggressive const-prop' implementation. This commit restricts const-prop' for such cases where the non-const call result is limited to avoid the issue. This fix is conservative, but given that accurate inference is mostly impossible when there are unresolvable cycles (which is indicated by limited result), aggressive const-prop' isn't necessary for such cases, and I don't anticipate this leading to any obvious regression. fix #52763 --- base/compiler/abstractinterpretation.jl | 54 +++++++++++++------------ test/compiler/inference.jl | 13 +++++- 2 files changed, 41 insertions(+), 26 deletions(-) diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl index 9acddad45721a..ab1d8c7c570f9 100644 --- a/base/compiler/abstractinterpretation.jl +++ b/base/compiler/abstractinterpretation.jl @@ -785,11 +785,7 @@ end function abstract_call_method_with_const_args(interp::AbstractInterpreter, result::MethodCallResult, @nospecialize(f), arginfo::ArgInfo, si::StmtInfo, match::MethodMatch, sv::AbsIntState, invokecall::Union{Nothing,InvokeCall}=nothing) - if !const_prop_enabled(interp, sv, match) - return nothing - end - if bail_out_const_call(interp, result, si) - add_remark!(interp, sv, "[constprop] No more information to be gained") + if !const_prop_enabled(interp, match, sv) || bail_out_const_call(interp, result, si, sv) return nothing end eligibility = concrete_eval_eligible(interp, f, result, arginfo, sv) @@ -822,7 +818,7 @@ function abstract_call_method_with_const_args(interp::AbstractInterpreter, return const_prop_call(interp, mi, result, arginfo, sv, concrete_eval_result) end -function const_prop_enabled(interp::AbstractInterpreter, sv::AbsIntState, match::MethodMatch) +function const_prop_enabled(interp::AbstractInterpreter, match::MethodMatch, sv::AbsIntState) if !InferenceParams(interp).ipo_constant_propagation add_remark!(interp, sv, "[constprop] Disabled by parameter") return false @@ -834,9 +830,11 @@ function const_prop_enabled(interp::AbstractInterpreter, sv::AbsIntState, match: return true end -function bail_out_const_call(interp::AbstractInterpreter, result::MethodCallResult, si::StmtInfo) +function bail_out_const_call(interp::AbstractInterpreter, result::MethodCallResult, + si::StmtInfo, sv::AbsIntState) if is_removable_if_unused(result.effects) if isa(result.rt, Const) || call_result_unused(si) + add_remark!(interp, sv, "[constprop] No more information to be gained (const)") return true end end @@ -937,7 +935,10 @@ function maybe_get_const_prop_profitable(interp::AbstractInterpreter, match::MethodMatch, sv::AbsIntState) method = match.method force = force_const_prop(interp, f, method) - force || const_prop_entry_heuristic(interp, result, si, sv) || return nothing + if !const_prop_entry_heuristic(interp, result, si, sv, force) + # N.B. remarks are emitted within `const_prop_entry_heuristic` + return nothing + end nargs::Int = method.nargs method.isva && (nargs -= 1) length(arginfo.argtypes) < nargs && return nothing @@ -964,8 +965,17 @@ function maybe_get_const_prop_profitable(interp::AbstractInterpreter, return mi end -function const_prop_entry_heuristic(interp::AbstractInterpreter, result::MethodCallResult, si::StmtInfo, sv::AbsIntState) - if call_result_unused(si) && result.edgecycle +function const_prop_entry_heuristic(interp::AbstractInterpreter, result::MethodCallResult, + si::StmtInfo, sv::AbsIntState, force::Bool) + if result.rt isa LimitedAccuracy + # optimizations like inlining are disabled for limited frames, + # thus there won't be much benefit in constant-prop' here + # N.B. don't allow forced constprop' for safety (xref #52763) + add_remark!(interp, sv, "[constprop] Disabled by entry heuristic (limited accuracy)") + return false + elseif force + return true + elseif call_result_unused(si) && result.edgecycle add_remark!(interp, sv, "[constprop] Disabled by entry heuristic (edgecycle with unused result)") return false end @@ -978,27 +988,21 @@ function const_prop_entry_heuristic(interp::AbstractInterpreter, result::MethodC if rt === Bottom add_remark!(interp, sv, "[constprop] Disabled by entry heuristic (erroneous result)") return false - else - return true end + return true elseif isa(rt, PartialStruct) || isa(rt, InterConditional) || isa(rt, InterMustAlias) # could be improved to `Const` or a more precise wrapper return true - elseif isa(rt, LimitedAccuracy) - # optimizations like inlining are disabled for limited frames, - # thus there won't be much benefit in constant-prop' here - add_remark!(interp, sv, "[constprop] Disabled by entry heuristic (limited accuracy)") - return false - else - if isa(rt, Const) - if !is_nothrow(result.effects) - # Could still be improved to Bottom (or at least could see the effects improved) - return true - end + elseif isa(rt, Const) + if is_nothrow(result.effects) + add_remark!(interp, sv, "[constprop] Disabled by entry heuristic (nothrow const)") + return false end - add_remark!(interp, sv, "[constprop] Disabled by entry heuristic (unimprovable result)") - return false + # Could still be improved to Bottom (or at least could see the effects improved) + return true end + add_remark!(interp, sv, "[constprop] Disabled by entry heuristic (unimprovable result)") + return false end # determines heuristically whether if constant propagation can be worthwhile diff --git a/test/compiler/inference.jl b/test/compiler/inference.jl index cd883bb70884e..0fbf4e9b0fd7d 100644 --- a/test/compiler/inference.jl +++ b/test/compiler/inference.jl @@ -4845,10 +4845,21 @@ g() = empty_nt_values(Base.inferencebarrier(Tuple{})) # This is somewhat sensitive to the exact recursion level that inference is willing to do, but the intention # is to test the case where inference limited a recursion, but then a forced constprop nevertheless managed # to terminate the call. +@newinterp RecurseInterpreter +let CC = Core.Compiler + function CC.const_prop_entry_heuristic(interp::RecurseInterpreter, result::CC.MethodCallResult, + si::CC.StmtInfo, sv::CC.AbsIntState, force::Bool) + if result.rt isa CC.LimitedAccuracy + return force # allow forced constprop to recurse into unresolved cycles + end + return @invoke CC.const_prop_entry_heuristic(interp::CC.AbstractInterpreter, result::CC.MethodCallResult, + si::CC.StmtInfo, sv::CC.AbsIntState, force::Bool) + end +end Base.@constprop :aggressive type_level_recurse1(x...) = x[1] == 2 ? 1 : (length(x) > 100 ? x : type_level_recurse2(x[1] + 1, x..., x...)) Base.@constprop :aggressive type_level_recurse2(x...) = type_level_recurse1(x...) type_level_recurse_entry() = Val{type_level_recurse1(1)}() -@test Base.return_types(type_level_recurse_entry, ()) |> only == Val{1} +@test only(Base.return_types(type_level_recurse_entry, (); interp=RecurseInterpreter())) == Val{1} # Test that inference doesn't give up if it can potentially refine effects, # even if the return type is Any.