Skip to content

Commit

Permalink
Put back getfield :boundscheck hack (#48677)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Keno committed Feb 15, 2023
1 parent afeda9f commit 113c2f3
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 2 deletions.
18 changes: 18 additions & 0 deletions base/compiler/abstractinterpretation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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"
Expand Down
8 changes: 6 additions & 2 deletions test/compiler/effects.jl
Original file line number Diff line number Diff line change
Expand Up @@ -739,15 +739,19 @@ 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

@test Base.infer_effects(Tuple{Int64}) do i
@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}
Expand Down

0 comments on commit 113c2f3

Please sign in to comment.