From 113c2f3c95bf2ae67ac1e8214a17b7d9404dbbfa Mon Sep 17 00:00:00 2001 From: Keno Fischer Date: Tue, 14 Feb 2023 19:51:12 -0500 Subject: [PATCH] Put back getfield :boundscheck hack (#48677) We used to have this hack before #48246, but I removed it because I had hoped we don't need. Unfortunately, we weren't able to infer consistency of ``` @inbounds (1,2)[1] ``` With this hack, constprop is able to independently prove inbounded-ness, overriding the usual consistency taintaing that happens for inbounds. --- base/compiler/abstractinterpretation.jl | 18 ++++++++++++++++++ test/compiler/effects.jl | 8 ++++++-- 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl index 27c88c7a87562..2fabbc18a83f9 100644 --- a/base/compiler/abstractinterpretation.jl +++ b/base/compiler/abstractinterpretation.jl @@ -1974,6 +1974,14 @@ function abstract_call_known(interp::AbstractInterpreter, @nospecialize(f), end rt = abstract_call_builtin(interp, f, arginfo, sv, max_methods) effects = builtin_effects(𝕃ᵢ, f, arginfo, rt) + if f === getfield && (fargs !== nothing && isexpr(fargs[end], :boundscheck)) && !is_nothrow(effects) && isa(sv, InferenceState) + # As a special case, we delayed tainting `noinbounds` for getfield calls in case we can prove + # in-boundedness indepedently. Here we need to put that back in other cases. + # N.B.: This isn't about the effects of the call itself, but a delayed contribution of the :boundscheck + # statement, so we need to merge this directly into sv, rather than modifying thte effects. + merge_effects!(interp, sv, Effects(EFFECTS_TOTAL; noinbounds=false, + consistent = (get_curr_ssaflag(sv) & IR_FLAG_INBOUNDS) != 0 ? ALWAYS_FALSE : ALWAYS_TRUE)) + end return CallMeta(rt, effects, NoCallInfo()) elseif isa(f, Core.OpaqueClosure) # calling an OpaqueClosure about which we have no information returns no information @@ -2195,6 +2203,15 @@ function abstract_eval_value_expr(interp::AbstractInterpreter, e::Expr, vtypes:: return rt elseif head === :boundscheck if isa(sv, InferenceState) + stmt = sv.src.code[sv.currpc] + if isexpr(stmt, :call) + f = abstract_eval_value(interp, stmt.args[1], vtypes, sv) + if f isa Const && f.val === getfield + # boundscheck of `getfield` call is analyzed by tfunc potentially without + # tainting :inbounds or :consistent when it's known to be nothrow + @goto delay_effects_analysis + end + end # If there is no particular `@inbounds` for this function, then we only taint `:noinbounds`, # which will subsequently taint `:consistent`-cy if this function is called from another # function that uses `@inbounds`. However, if this `:boundscheck` is itself within an @@ -2203,6 +2220,7 @@ function abstract_eval_value_expr(interp::AbstractInterpreter, e::Expr, vtypes:: merge_effects!(interp, sv, Effects(EFFECTS_TOTAL; noinbounds=false, consistent = (get_curr_ssaflag(sv) & IR_FLAG_INBOUNDS) != 0 ? ALWAYS_FALSE : ALWAYS_TRUE)) end + @label delay_effects_analysis rt = Bool elseif head === :inbounds @assert false && "Expected this to have been moved into flags" diff --git a/test/compiler/effects.jl b/test/compiler/effects.jl index 599da1225cf52..dceb737ed6ae7 100644 --- a/test/compiler/effects.jl +++ b/test/compiler/effects.jl @@ -739,8 +739,8 @@ end |> Core.Compiler.is_foldable_nothrow # Test that dead `@inbounds` does not taint consistency # https://github.com/JuliaLang/julia/issues/48243 -@test Base.infer_effects() do - false && @inbounds (1,2,3)[1] +@test Base.infer_effects(Tuple{Int64}) do i + false && @inbounds (1,2,3)[i] return 1 end |> Core.Compiler.is_foldable_nothrow @@ -748,6 +748,10 @@ end |> Core.Compiler.is_foldable_nothrow @inbounds (1,2,3)[i] end |> !Core.Compiler.is_consistent +@test Base.infer_effects(Tuple{Tuple{Int64}}) do x + @inbounds x[1] +end |> Core.Compiler.is_foldable_nothrow + # Test that :new of non-concrete, but otherwise known type # does not taint consistency. @eval struct ImmutRef{T}