Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow irinterp to refine nothrow effect #48066

Merged
merged 1 commit into from
Jan 6, 2023
Merged

Allow irinterp to refine nothrow effect #48066

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions base/compiler/abstractinterpretation.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -836,9 +836,7 @@ function concrete_eval_eligible(interp::AbstractInterpreter,
if is_all_const_arg(arginfo, #=start=#2)
return true
else
# TODO: `is_nothrow` is not an actual requirement here, this is just a hack
# to avoid entering semi concrete eval while it doesn't properly override effects
return is_nothrow(result.effects) ? false : nothing
return false
end
end
return nothing
Expand Down Expand Up @@ -1010,10 +1008,11 @@ function abstract_call_method_with_const_args(interp::AbstractInterpreter,
ir = codeinst_to_ir(interp, code)
if isa(ir, IRCode)
irsv = IRInterpretationState(interp, ir, mi, sv.world, arginfo.argtypes)
rt = ir_abstract_constant_propagation(interp, irsv)
rt, nothrow = ir_abstract_constant_propagation(interp, irsv)
@assert !(rt isa Conditional || rt isa MustAlias) "invalid lattice element returned from IR interpretation"
if !isa(rt, Type) || typeintersect(rt, Bool) === Union{}
return ConstCallResults(rt, SemiConcreteResult(mi, ir, result.effects), result.effects, mi)
new_effects = Effects(result.effects; nothrow=nothrow)
return ConstCallResults(rt, SemiConcreteResult(mi, ir, new_effects), new_effects, mi)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion base/compiler/optimize.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -649,7 +649,7 @@ function convert_to_ircode(ci::CodeInfo, sv::OptimizationState)
insert!(codelocs, idx + 1, codelocs[idx])
insert!(ssavaluetypes, idx + 1, Union{})
insert!(stmtinfo, idx + 1, NoCallInfo())
insert!(ssaflags, idx + 1, ssaflags[idx])
insert!(ssaflags, idx + 1, IR_FLAG_NOTHROW)
if ssachangemap === nothing
ssachangemap = fill(0, nstmts)
end
Expand Down
40 changes: 30 additions & 10 deletions base/compiler/ssair/irinterp.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,20 +144,20 @@ function concrete_eval_invoke(interp::AbstractInterpreter,
inst::Expr, mi::MethodInstance, irsv::IRInterpretationState)
mi_cache = WorldView(code_cache(interp), irsv.world)
code = get(mi_cache, mi, nothing)
code === nothing && return nothing
code === nothing && return Pair{Any, Bool}(nothing, false)
argtypes = collect_argtypes(interp, inst.args[2:end], nothing, irsv.ir)
argtypes === nothing && return Union{}
argtypes === nothing && return Pair{Any, Bool}(Union{}, false)
effects = decode_effects(code.ipo_purity_bits)
if is_foldable(effects) && is_all_const_arg(argtypes, #=start=#1)
args = collect_const_args(argtypes, #=start=#1)
world = get_world_counter(interp)
value = try
Core._call_in_world_total(world, args...)
catch
return Union{}
return Pair{Any, Bool}(Union{}, false)
end
if is_inlineable_constant(value)
return Const(value)
return Pair{Any, Bool}(Const(value), true)
end
else
ir′ = codeinst_to_ir(interp, code)
Expand All @@ -166,7 +166,7 @@ function concrete_eval_invoke(interp::AbstractInterpreter,
return _ir_abstract_constant_propagation(interp, irsv′)
end
end
return nothing
return Pair{Any, Bool}(nothing, is_nothrow(effects))
end

function abstract_eval_phi_stmt(interp::AbstractInterpreter, phi::PhiNode, ::Int, irsv::IRInterpretationState)
Expand All @@ -183,6 +183,12 @@ function reprocess_instruction!(interp::AbstractInterpreter,
if condval isa Bool
function update_phi!(from::Int, to::Int)
if length(ir.cfg.blocks[to].preds) == 0
# Kill the entire block
for idx in ir.cfg.blocks[to].stmts
ir.stmts[idx][:inst] = nothing
ir.stmts[idx][:type] = Union{}
ir.stmts[idx][:flag] = IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW
end
return
end
for idx in ir.cfg.blocks[to].stmts
Expand All @@ -205,6 +211,7 @@ function reprocess_instruction!(interp::AbstractInterpreter,
if bb === nothing
bb = block_for_inst(ir, idx)
end
ir.stmts[idx][:flag] |= IR_FLAG_NOTHROW
if condval
ir.stmts[idx][:inst] = nothing
ir.stmts[idx][:type] = Any
Expand All @@ -221,20 +228,25 @@ function reprocess_instruction!(interp::AbstractInterpreter,
rt = nothing
if isa(inst, Expr)
head = inst.head
if head === :call || head === :foreigncall || head === :new
if head === :call || head === :foreigncall || head === :new || head === :splatnew
(; rt, effects) = abstract_eval_statement_expr(interp, inst, nothing, ir, irsv.mi)
# All other effects already guaranteed effect free by construction
if is_nothrow(effects)
ir.stmts[idx][:flag] |= IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IR_FLAG_EFFECT_FREE seems a bit risky here since we may prove :effect_free-ness in a presence of effect-ful statement, e.g.

julia> mutable struct SafeRef{T}
           x::T
       end

julia> Base.getindex(x::SafeRef) = x.x

julia> Base.setindex!(x::SafeRef, y) = x.x = y

julia> Base.infer_effects((String,String,)) do x, y
           r = SafeRef(x)
           r[] = y
           r[]
       end
(+c,+e,+n,+t,+s,+m)

So I guess we need to check is_removable_if_unused here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're probably right, though IR_FLAG_EFFECT_FREE was there before, so this might be an existing bug. Ideally we should have a testcase for this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attempted to create a test case to trigger an error because of this issue, but it seems very difficult to invent one. Something like the example below would be close (it actually crashes due to a different bug in our SROA though).

Base.@assume_effects :consistent :nothrow @inline function foo(a::Int, b)
    r = Ref(a)
    try
        setfield!(r, :x, b)
        nothing
    catch err
        return isa(b, String)
    end
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xref: #48067

if isa(rt, Const) && is_inlineable_constant(rt.val)
ir.stmts[idx][:inst] = quoted(rt.val)
else
ir.stmts[idx][:flag] |= IR_FLAG_EFFECT_FREE
end
end
elseif head === :invoke
mi′ = inst.args[1]::MethodInstance
if mi′ !== irsv.mi # prevent infinite loop
rt = concrete_eval_invoke(interp, inst, mi′, irsv)
rt, nothrow = concrete_eval_invoke(interp, inst, mi′, irsv)
if nothrow
ir.stmts[idx][:flag] |= IR_FLAG_EFFECT_FREE | IR_FLAG_NOTHROW
if isa(rt, Const) && is_inlineable_constant(rt.val)
ir.stmts[idx][:inst] = quoted(rt.val)
end
end
end
elseif head === :throw_undef_if_not || # TODO: Terminate interpretation early if known false?
head === :gc_preserve_begin ||
Expand Down Expand Up @@ -416,7 +428,15 @@ function _ir_abstract_constant_propagation(interp::AbstractInterpreter, irsv::IR
end
end

return maybe_singleton_const(ultimate_rt)
nothrow = true
for i = 1:length(ir.stmts)
if (ir.stmts[i][:flag] & IR_FLAG_NOTHROW) == 0
nothrow = false
break
end
end

return Pair{Any, Bool}(maybe_singleton_const(ultimate_rt), nothrow)
end

function ir_abstract_constant_propagation(interp::AbstractInterpreter, irsv::IRInterpretationState)
Expand Down
2 changes: 1 addition & 1 deletion base/compiler/ssair/slot2ssa.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ function strip_trailing_junk!(ci::CodeInfo, code::Vector{Any}, info::Vector{Call
push!(ssavaluetypes, Union{})
push!(codelocs, 0)
push!(info, NoCallInfo())
push!(ssaflags, IR_FLAG_NULL)
push!(ssaflags, IR_FLAG_NOTHROW)
end
nothing
end
Expand Down
2 changes: 2 additions & 0 deletions base/reflection.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,6 +786,7 @@ julia> Base.fieldindex(Foo, :z, false)
"""
function fieldindex(T::DataType, name::Symbol, err::Bool=true)
@_foldable_meta
@noinline
return Int(ccall(:jl_field_index, Cint, (Any, Any, Cint), T, name, err)+1)
end

Expand All @@ -800,6 +801,7 @@ end

function argument_datatype(@nospecialize t)
@_total_meta
@noinline
return ccall(:jl_argument_datatype, Any, (Any,), t)::Union{Nothing,DataType}
end

Expand Down
1 change: 1 addition & 0 deletions base/tuple.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,6 +581,7 @@ any(x::Tuple{Bool, Bool, Bool}) = x[1]|x[2]|x[3]

# a version of `in` esp. for NamedTuple, to make it pure, and not compiled for each tuple length
function sym_in(x::Symbol, @nospecialize itr::Tuple{Vararg{Symbol}})
@noinline
@_total_meta
for y in itr
y === x && return true
Expand Down
2 changes: 1 addition & 1 deletion test/broadcast.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1104,7 +1104,7 @@ end
end
arr = rand(1000)
@allocated test(arr)
@test (@allocated test(arr)) == 0
@test (@allocated test(arr)) <= 16
end

@testset "Fix type unstable .&& #43470" begin
Expand Down