From c9c0c4d1491276887927d9fcb986e270e9bea961 Mon Sep 17 00:00:00 2001 From: Shuhei Kadowaki Date: Mon, 11 Jul 2022 20:32:14 +0900 Subject: [PATCH 1/2] effects: relax recursion detection for effects analysis In a similar spirit to #40561, we can relax the recursion detection to guarantee `:terminates` effect and allow the effects analysis to not taint `:terminates` effect when there are no cycles in `MethodInstance`s in a call graph. fix #45781 --- base/compiler/abstractinterpretation.jl | 53 ++++++++++++++++++------- test/compiler/effects.jl | 15 +++++++ 2 files changed, 54 insertions(+), 14 deletions(-) diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl index 5aa7669c3a3a9..121f7b57129f0 100644 --- a/base/compiler/abstractinterpretation.jl +++ b/base/compiler/abstractinterpretation.jl @@ -617,9 +617,13 @@ function abstract_call_method(interp::AbstractInterpreter, method::Method, @nosp # this edge is known to terminate effects = Effects(effects; terminates=ALWAYS_TRUE) elseif edgecycle - # Some sort of recursion was detected. Even if we did not limit types, - # we cannot guarantee that the call will terminate - effects = Effects(effects; terminates=ALWAYS_FALSE) + # Some sort of recursion was detected. + if edge !== nothing && !edgelimited && !is_edge_recursed(edge, sv) + # no `MethodInstance` cycles -- don't taint :terminate + else + # we cannot guarantee that the call will terminate + effects = Effects(effects; terminates=ALWAYS_FALSE) + end end return MethodCallResult(rt, edgecycle, edgelimited, edge, effects) @@ -691,6 +695,30 @@ function matches_sv(parent::InferenceState, sv::InferenceState) return parent.linfo.def === sv.linfo.def && sv_method2 === parent_method2 end +function is_edge_recursed(edge::MethodInstance, sv::InferenceState) + return any(InfStackUnwind(sv)) do infstate + return edge === infstate.linfo + end +end + +function is_method_recursed(method::Method, sv::InferenceState) + return any(InfStackUnwind(sv)) do infstate + return method === infstate.linfo.def + end +end + +function is_constprop_edge_recursed(edge::MethodInstance, sv::InferenceState) + return any(InfStackUnwind(sv)) do infstate + return edge === infstate.linfo && any(infstate.result.overridden_by_const) + end +end + +function is_constprop_method_recursed(method::Method, sv::InferenceState) + return any(InfStackUnwind(sv)) do infstate + return method === infstate.linfo.def && any(infstate.result.overridden_by_const) + end +end + # keeps result and context information of abstract_method_call, which will later be used for # backedge computation, and concrete evaluation or constant-propagation struct MethodCallResult @@ -836,17 +864,14 @@ function abstract_call_method_with_const_args(interp::AbstractInterpreter, resul if inf_result === nothing # if there might be a cycle, check to make sure we don't end up # calling ourselves here. - let result = result # prevent capturing - if result.edgecycle && _any(InfStackUnwind(sv)) do infstate - # if the type complexity limiting didn't decide to limit the call signature (`result.edgelimited = false`) - # we can relax the cycle detection by comparing `MethodInstance`s and allow inference to - # propagate different constant elements if the recursion is finite over the lattice - return (result.edgelimited ? match.method === infstate.linfo.def : mi === infstate.linfo) && - any(infstate.result.overridden_by_const) - end - add_remark!(interp, sv, "[constprop] Edge cycle encountered") - return nothing - end + if result.edgecycle && (result.edgelimited ? + is_constprop_method_recursed(match.method, sv) : + # if the type complexity limiting didn't decide to limit the call signature (`result.edgelimited = false`) + # we can relax the cycle detection by comparing `MethodInstance`s and allow inference to + # propagate different constant elements if the recursion is finite over the lattice + is_constprop_edge_recursed(mi, sv)) + add_remark!(interp, sv, "[constprop] Edge cycle encountered") + return nothing end inf_result = InferenceResult(mi, (arginfo, sv)) if !any(inf_result.overridden_by_const) diff --git a/test/compiler/effects.jl b/test/compiler/effects.jl index 960ea2de817be..d63b99bf81ace 100644 --- a/test/compiler/effects.jl +++ b/test/compiler/effects.jl @@ -6,6 +6,21 @@ include("irutils.jl") for i = 1:n; end end |> !Core.Compiler.is_terminates +# interprocedural-recursion should taint `terminates` **appropriately** +function sumrecur(a, x) + isempty(a) && return x + return sumrecur(Base.tail(a), x + first(a)) +end +@test Base.infer_effects(sumrecur, (Tuple{Int,Int,Int},Int)) |> Core.Compiler.is_terminates +@test Base.infer_effects(sumrecur, (Tuple{Int,Int,Int,Vararg{Int}},Int)) |> !Core.Compiler.is_terminates + +# https://github.com/JuliaLang/julia/issues/45781 +@test Base.infer_effects((Float32,)) do a + out1 = promote_type(Irrational{:π}, Bool) + out2 = sin(a) + out1, out2 +end |> Core.Compiler.is_terminates + # refine :consistent-cy effect inference using the return type information @test Base.infer_effects((Any,)) do x taint = Ref{Any}(x) # taints :consistent-cy, but will be adjusted From 703b12ca3a2bc3ba67099367a8c93e19467acb22 Mon Sep 17 00:00:00 2001 From: Shuhei Kadowaki Date: Mon, 11 Jul 2022 20:42:13 +0900 Subject: [PATCH 2/2] minor NFC changes --- base/compiler/abstractinterpretation.jl | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl index 121f7b57129f0..e2802768d98ad 100644 --- a/base/compiler/abstractinterpretation.jl +++ b/base/compiler/abstractinterpretation.jl @@ -988,8 +988,8 @@ function is_const_prop_profitable_arg(@nospecialize(arg)) isa(arg, PartialOpaque) && return true isa(arg, Const) || return true val = arg.val - # don't consider mutable values or Strings useful constants - return isa(val, Symbol) || isa(val, Type) || (!isa(val, String) && !ismutable(val)) + # don't consider mutable values useful constants + return isa(val, Symbol) || isa(val, Type) || !ismutable(val) end function is_const_prop_profitable_conditional(cnd::Conditional, fargs::Vector{Any}, sv::InferenceState) @@ -1370,7 +1370,7 @@ function abstract_apply(interp::AbstractInterpreter, argtypes::Vector{Any}, sv:: end cti = Any[Vararg{argt}] end - if _any(t -> t === Bottom, cti) + if any(@nospecialize(t) -> t === Bottom, cti) continue end for j = 1:length(ctypes) @@ -2056,6 +2056,8 @@ function abstract_eval_statement(interp::AbstractInterpreter, @nospecialize(e), for i = 3:length(e.args) if abstract_eval_value(interp, e.args[i], vtypes, sv) === Bottom t = Bottom + tristate_merge!(sv, EFFECTS_THROWS) + @goto t_computed end end cconv = e.args[5]