Rework :inbounds effects tainting (#48246)

This works to fix #48243, by only tanting effects if an `@inbounds` statement
is actually reached. Further, it refines the `noinbounds` effect to be
IPO-cached and used to track whether a particular method read the inbounds state.
A `:boundscheck` expression now does not immediately taint consistencty, but
instead, taints `noinbounds` only. Then, if a method that has `:noinbounds`
tainted is called within an `@inbounds` region, consistency is tainted.
Similarly, a tainted `:noinbounds` disables constant propagation at
`@inbounds` statements or if the method propagates inbounds.

(cherry picked from commit d544e78)

base/compiler/abstractinterpretation.jl

@@ -781,9 +781,18 @@ end
 # - false: eligible for semi-concrete evaluation
 # - nothing: not eligible for either of it
 function concrete_eval_eligible(interp::AbstractInterpreter,
-    @nospecialize(f), result::MethodCallResult, arginfo::ArgInfo)
+    @nospecialize(f), result::MethodCallResult, arginfo::ArgInfo, sv::InferenceState)
     # disable all concrete-evaluation if this function call is tainted by some overlayed
     # method since currently there is no direct way to execute overlayed methods
+    if inbounds_option() === :off
+        # Disable concrete evaluation in `--check-bounds=no` mode, since we cannot be sure
+        # that inferred effects are accurate.
+        return nothing
+    elseif !result.effects.noinbounds && stmt_taints_inbounds_consistency(sv)
+        # If the current statement is @inbounds or we propagate inbounds, the call's consistency
+        # is tainted and not consteval eligible.
+        return nothing
+    end
     isoverlayed(method_table(interp)) && !is_nonoverlayed(result.effects) && return nothing
     if f !== nothing && result.edge !== nothing && is_foldable(result.effects)
         if is_all_const_arg(arginfo, #=start=#2)
@@ -824,7 +833,7 @@ end
 function concrete_eval_call(interp::AbstractInterpreter,
     @nospecialize(f), result::MethodCallResult, arginfo::ArgInfo, si::StmtInfo,
     sv::InferenceState, invokecall::Union{Nothing,InvokeCall}=nothing)
-    eligible = concrete_eval_eligible(interp, f, result, arginfo)
+    eligible = concrete_eval_eligible(interp, f, result, arginfo, sv)
     eligible === nothing && return false
     if eligible
         args = collect_const_args(arginfo, #=start=#2)
@@ -2036,21 +2045,18 @@ function abstract_eval_value_expr(interp::AbstractInterpreter, e::Expr, vtypes::
         end
     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 :consistent-cy when it's known to be nothrow
-                    @goto delay_effects_analysis
-                end
-            end
+            flag = sv.src.ssaflags[sv.currpc]
+            # If there is no particular @inbounds for this function, then we only taint `noinbounds`,
+            # which will subsequently taint consistency if this function is called from another
+            # function that uses `@inbounds`. However, if this :boundscheck is itself within an
+            # `@inbounds` region, its value depends on `--check-bounds`, so we need to taint
+            # consistency here also.
+            merge_effects!(interp, sv, Effects(EFFECTS_TOTAL; noinbounds=false,
+                consistent = (flag & IR_FLAG_INBOUNDS) != 0 ? ALWAYS_FALSE : ALWAYS_TRUE))
         end
-        merge_effects!(interp, sv, Effects(EFFECTS_TOTAL; consistent=ALWAYS_FALSE, noinbounds=false))
-        @label delay_effects_analysis
         rt = Bool
     elseif head === :inbounds
-        merge_effects!(interp, sv, Effects(EFFECTS_TOTAL; consistent=ALWAYS_FALSE, noinbounds=false))
+        @assert false && "Expected this to have been moved into flags"
     elseif head === :the_exception
         merge_effects!(interp, sv, Effects(EFFECTS_TOTAL; consistent=ALWAYS_FALSE))
     end
@@ -2125,7 +2131,6 @@ function abstract_eval_statement_expr(interp::AbstractInterpreter, e::Expr, vtyp
             arginfo = ArgInfo(ea, argtypes)
             si = StmtInfo(isa(sv, IRCode) ? true : !call_result_unused(sv, sv.currpc))
             (; rt, effects, info) = abstract_call(interp, arginfo, si, sv)
-            merge_effects!(interp, sv, effects)
             if isa(sv, InferenceState)
                 sv.stmt_info[sv.currpc] = info
             end
@@ -2189,7 +2194,6 @@ function abstract_eval_statement_expr(interp::AbstractInterpreter, e::Expr, vtyp
             nothrow = false
         end
         effects = Effects(EFFECTS_TOTAL; consistent, nothrow)
-        merge_effects!(interp, sv, effects)
     elseif ehead === :splatnew
         t, isexact = instanceof_tfunc(abstract_eval_value(interp, e.args[1], vtypes, sv))
         nothrow = false # TODO: More precision
@@ -2208,7 +2212,6 @@ function abstract_eval_statement_expr(interp::AbstractInterpreter, e::Expr, vtyp
         end
         consistent = !ismutabletype(t) ? ALWAYS_TRUE : CONSISTENT_IF_NOTRETURNED
         effects = Effects(EFFECTS_TOTAL; consistent, nothrow)
-        merge_effects!(interp, sv, effects)
     elseif ehead === :new_opaque_closure
         t = Union{}
         effects = Effects() # TODO
@@ -2236,20 +2239,16 @@ function abstract_eval_statement_expr(interp::AbstractInterpreter, e::Expr, vtyp
     elseif ehead === :foreigncall
         (;rt, effects) = abstract_eval_foreigncall(interp, e, vtypes, sv, mi)
         t = rt
-        merge_effects!(interp, sv, effects)
     elseif ehead === :cfunction
         effects = EFFECTS_UNKNOWN
-        merge_effects!(interp, sv, effects)
         t = e.args[1]
         isa(t, Type) || (t = Any)
         abstract_eval_cfunction(interp, e, vtypes, sv)
     elseif ehead === :method
         t = (length(e.args) == 1) ? Any : Nothing
         effects = EFFECTS_UNKNOWN
-        merge_effects!(interp, sv, effects)
     elseif ehead === :copyast
         effects = EFFECTS_UNKNOWN
-        merge_effects!(interp, sv, effects)
         t = abstract_eval_value(interp, e.args[1], vtypes, sv)
         if t isa Const && t.val isa Expr
             # `copyast` makes copies of Exprs
@@ -2260,6 +2259,7 @@ function abstract_eval_statement_expr(interp::AbstractInterpreter, e::Expr, vtyp
     elseif ehead === :isdefined
         sym = e.args[1]
         t = Bool
+        effects = EFFECTS_TOTAL
         if isa(sym, SlotNumber)
             vtyp = vtypes[slot_id(sym)]
             if vtyp.typ === Bottom
@@ -2288,9 +2288,9 @@ function abstract_eval_statement_expr(interp::AbstractInterpreter, e::Expr, vtyp
         @label always_throw
         t = Bottom
         effects = EFFECTS_THROWS
-        merge_effects!(interp, sv, effects)
     else
         t = abstract_eval_value_expr(interp, e, vtypes, sv)
+        effects = EFFECTS_TOTAL
     end
     return RTEffects(t, effects)
 end
@@ -2332,6 +2332,11 @@ function abstract_eval_phi(interp::AbstractInterpreter, phi::PhiNode, vtypes::Un
     return rt
 end
 
+function stmt_taints_inbounds_consistency(sv::InferenceState)
+    flag = sv.src.ssaflags[sv.currpc]
+    return sv.src.propagate_inbounds || (flag & IR_FLAG_INBOUNDS) != 0
+end
+
 function abstract_eval_statement(interp::AbstractInterpreter, @nospecialize(e), vtypes::VarTable, sv::InferenceState)
     if !isa(e, Expr)
         if isa(e, PhiNode)
@@ -2340,6 +2345,18 @@ function abstract_eval_statement(interp::AbstractInterpreter, @nospecialize(e),
         return abstract_eval_special_value(interp, e, vtypes, sv)
     end
     (;rt, effects) = abstract_eval_statement_expr(interp, e, vtypes, sv, nothing)
+    if !effects.noinbounds
+        flag = sv.src.ssaflags[sv.currpc]
+        if !sv.src.propagate_inbounds
+            # The callee read our inbounds flag, but unless we propagate inbounds,
+            # we ourselves don't read our parent's inbounds.
+            effects = Effects(effects; noinbounds=true)
+        end
+        if (flag & IR_FLAG_INBOUNDS) != 0
+            effects = Effects(effects; consistent=ALWAYS_FALSE)
+        end
+    end
+    merge_effects!(interp, sv, effects)
     e = e::Expr
     @assert !isa(rt, TypeVar) "unhandled TypeVar"
     rt = maybe_singleton_const(rt)

base/compiler/effects.jl

@@ -40,9 +40,9 @@ following meanings:
     This state corresponds to LLVM's `inaccessiblemem_or_argmemonly` function attribute.
 - `nonoverlayed::Bool`: indicates that any methods that may be called within this method
   are not defined in an [overlayed method table](@ref OverlayMethodTable).
-- `noinbounds::Bool`: indicates this method can't be `:consistent` because of bounds checking.
-  This effect is currently only set on `InferenceState` construction and used to taint
-  `:consistent`-cy before caching. We may want to track it with more accuracy in the future.
+- `noinbounds::Bool`: If set, indicates that this method does not read the parent's :inbounds
+  state. In particular, it does not have any reached :boundscheck exprs, not propagates inbounds
+  to any children that do.
 
 Note that the representations above are just internal implementation details and thus likely
 to change in the future. See [`Base.@assume_effects`](@ref) for more detailed explanation
@@ -98,10 +98,10 @@ const EFFECT_FREE_IF_INACCESSIBLEMEMONLY = 0x01 << 1
 # :inaccessiblememonly bits
 const INACCESSIBLEMEM_OR_ARGMEMONLY = 0x01 << 1
 
-const EFFECTS_TOTAL    = Effects(ALWAYS_TRUE,  ALWAYS_TRUE,  true,  true,  true,  ALWAYS_TRUE,  true)
-const EFFECTS_THROWS   = Effects(ALWAYS_TRUE,  ALWAYS_TRUE,  false, true,  true,  ALWAYS_TRUE,  true)
-const EFFECTS_UNKNOWN  = Effects(ALWAYS_FALSE, ALWAYS_FALSE, false, false, false, ALWAYS_FALSE, true)  # unknown mostly, but it's not overlayed at least (e.g. it's not a call)
-const EFFECTS_UNKNOWN′ = Effects(ALWAYS_FALSE, ALWAYS_FALSE, false, false, false, ALWAYS_FALSE, false) # unknown really
+const EFFECTS_TOTAL    = Effects(ALWAYS_TRUE,  ALWAYS_TRUE,  true,  true,  true,  ALWAYS_TRUE,  true, true)
+const EFFECTS_THROWS   = Effects(ALWAYS_TRUE,  ALWAYS_TRUE,  false, true,  true,  ALWAYS_TRUE,  true, true)
+const EFFECTS_UNKNOWN  = Effects(ALWAYS_FALSE, ALWAYS_FALSE, false, false, false, ALWAYS_FALSE, true, true)  # unknown mostly, but it's not overlayed at least (e.g. it's not a call)
+const EFFECTS_UNKNOWN′ = Effects(ALWAYS_FALSE, ALWAYS_FALSE, false, false, false, ALWAYS_FALSE, false, true) # unknown really
 
 function Effects(e::Effects = EFFECTS_UNKNOWN′;
     consistent::UInt8 = e.consistent,
@@ -184,7 +184,8 @@ function encode_effects(e::Effects)
            ((e.terminates          % UInt32) << 6) |
            ((e.notaskstate         % UInt32) << 7) |
            ((e.inaccessiblememonly % UInt32) << 8) |
-           ((e.nonoverlayed        % UInt32) << 10)
+           ((e.nonoverlayed        % UInt32) << 10)|
+           ((e.noinbounds          % UInt32) << 11)
 end
 
 function decode_effects(e::UInt32)
@@ -195,7 +196,8 @@ function decode_effects(e::UInt32)
         _Bool((e >> 6) & 0x01),
         _Bool((e >> 7) & 0x01),
         UInt8((e >> 8) & 0x03),
-        _Bool((e >> 10) & 0x01))
+        _Bool((e >> 10) & 0x01),
+        _Bool((e >> 11) & 0x01))
 end
 
 struct EffectsOverride

base/compiler/inferencestate.jl

@@ -177,16 +177,7 @@ mutable struct InferenceState
 
         valid_worlds = WorldRange(src.min_world, src.max_world == typemax(UInt) ? get_world_counter() : src.max_world)
         bestguess = Bottom
-        # TODO: Currently, any :inbounds declaration taints consistency,
-        #       because we cannot be guaranteed whether or not boundschecks
-        #       will be eliminated and if they are, we cannot be guaranteed
-        #       that no undefined behavior will occur (the effects assumptions
-        #       are stronger than the inbounds assumptions, since the latter
-        #       requires dynamic reachability, while the former is global).
-        inbounds = inbounds_option()
-        noinbounds = inbounds === :on || (inbounds === :default && all(flag::UInt8->iszero(flag&IR_FLAG_INBOUNDS), src.ssaflags))
-        consistent = noinbounds ? ALWAYS_TRUE : ALWAYS_FALSE
-        ipo_effects = Effects(EFFECTS_TOTAL; consistent, noinbounds)
+        ipo_effects = Effects(EFFECTS_TOTAL)
 
         params = InferenceParams(interp)
         restrict_abstract_call_sites = isa(linfo.def, Module)

base/compiler/ssair/show.jl

@@ -1012,6 +1012,8 @@ function Base.show(io::IO, e::Effects)
     printstyled(io, effectbits_letter(e, :notaskstate, 's'); color=effectbits_color(e, :notaskstate))
     print(io, ',')
     printstyled(io, effectbits_letter(e, :inaccessiblememonly, 'm'); color=effectbits_color(e, :inaccessiblememonly))
+    print(io, ',')
+    printstyled(io, effectbits_letter(e, :noinbounds, 'i'); color=effectbits_color(e, :noinbounds))
     print(io, ')')
     e.nonoverlayed || printstyled(io, '′'; color=:red)
 end

base/expr.jl

@@ -607,6 +607,10 @@ currently equivalent to the following `setting`s:
     however, that by the `:consistent`-cy requirements, any such annotated call
     must consistently throw given the same argument values.
 
+!!! note
+    An explict `@inbounds` annotation inside the function will also disable
+    constant propagation and not be overriden by :foldable.
+
 ---
 ## `:total`
 

test/compiler/effects.jl

@@ -680,3 +680,13 @@ mksparamunused(x) = (SparamUnused(x); nothing)
 let src = code_typed1(mksparamunused, (Any,))
     @test count(isnew, src.code) == 0
 end
+
+# Test that dead `@inbounds` does not taint consistency
+@test Base.infer_effects() do
+    false && @inbounds (1,2,3)[1]
+    return 1
+end |> Core.Compiler.is_total
+
+@test Base.infer_effects(Tuple{Int64}) do i
+    @inbounds (1,2,3)[i]
+end |> !Core.Compiler.is_consistent