Skip to content

inference: refine slot undef info within then branch of @isdefined #55545

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

Merged
merged 1 commit into from
Aug 27, 2024
Merged

inference: refine slot undef info within then branch of @isdefined #55545

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
16 changes: 10 additions & 6 deletions base/compiler/abstractinterpretation.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2733,6 +2733,8 @@ function abstract_eval_isdefined(interp::AbstractInterpreter, e::Expr, vtypes::U
rt = Const(false) # never assigned previously
elseif !vtyp.undef
rt = Const(true) # definitely assigned previously
else # form `Conditional` to refine `vtyp.undef` in the then branch
rt = Conditional(sym, vtyp.typ, vtyp.typ; isdefined=true)
end
elseif isa(sym, GlobalRef)
if InferenceParams(interp).assume_bindings_static
Expand Down Expand Up @@ -3205,7 +3207,7 @@ end
@inline function abstract_eval_basic_statement(interp::AbstractInterpreter,
@nospecialize(stmt), pc_vartable::VarTable, frame::InferenceState)
if isa(stmt, NewvarNode)
changes = StateUpdate(stmt.slot, VarState(Bottom, true), false)
changes = StateUpdate(stmt.slot, VarState(Bottom, true))
return BasicStmtChange(changes, nothing, Union{})
elseif !isa(stmt, Expr)
(; rt, exct) = abstract_eval_statement(interp, stmt, pc_vartable, frame)
Expand All @@ -3220,7 +3222,7 @@ end
end
lhs = stmt.args[1]
if isa(lhs, SlotNumber)
changes = StateUpdate(lhs, VarState(rt, false), false)
changes = StateUpdate(lhs, VarState(rt, false))
elseif isa(lhs, GlobalRef)
handle_global_assignment!(interp, frame, lhs, rt)
elseif !isa(lhs, SSAValue)
Expand All @@ -3230,7 +3232,7 @@ end
elseif hd === :method
fname = stmt.args[1]
if isa(fname, SlotNumber)
changes = StateUpdate(fname, VarState(Any, false), false)
changes = StateUpdate(fname, VarState(Any, false))
end
return BasicStmtChange(changes, nothing, Union{})
elseif (hd === :code_coverage_effect || (
Expand Down Expand Up @@ -3576,7 +3578,7 @@ function apply_refinement!(𝕃ᵢ::AbstractLattice, slot::SlotNumber, @nospecia
oldtyp = vtype.typ
⊏ = strictpartialorder(𝕃ᵢ)
if newtyp ⊏ oldtyp
stmtupdate = StateUpdate(slot, VarState(newtyp, vtype.undef), false)
stmtupdate = StateUpdate(slot, VarState(newtyp, vtype.undef))
stoverwrite1!(currstate, stmtupdate)
end
end
Expand All @@ -3600,7 +3602,9 @@ function conditional_change(𝕃ᵢ::AbstractLattice, currstate::VarTable, condt
# "causes" since we ignored those in the comparison
newtyp = tmerge(𝕃ᵢ, newtyp, LimitedAccuracy(Bottom, oldtyp.causes))
end
return StateUpdate(SlotNumber(condt.slot), VarState(newtyp, vtype.undef), true)
# if this `Conditional` is from from `@isdefined condt.slot`, refine its `undef` information
newundef = condt.isdefined ? !then_or_else : vtype.undef
return StateUpdate(SlotNumber(condt.slot), VarState(newtyp, newundef), #=conditional=#true)
end

function condition_object_change(currstate::VarTable, condt::Conditional,
Expand All @@ -3609,7 +3613,7 @@ function condition_object_change(currstate::VarTable, condt::Conditional,
newcondt = Conditional(condt.slot,
then_or_else ? condt.thentype : Union{},
then_or_else ? Union{} : condt.elsetype)
return StateUpdate(condslot, VarState(newcondt, vtype.undef), false)
return StateUpdate(condslot, VarState(newcondt, vtype.undef))
end

# make as much progress on `frame` as possible (by handling cycles)
Expand Down
28 changes: 20 additions & 8 deletions base/compiler/typelattice.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,14 +73,19 @@ struct Conditional
slot::Int
thentype
elsetype
function Conditional(slot::Int, @nospecialize(thentype), @nospecialize(elsetype))
# `isdefined` indicates this `Conditional` is from `@isdefined slot`, implying that
# the `undef` information of `slot` can be improved in the then branch.
# Since this is only beneficial for local inference, it is not translated into `InterConditional`.
isdefined::Bool
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this works conceptually. isdefined-ness is not a flow property and can be reset by NewvarNode

Copy link
Member

Choose a reason for hiding this comment

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

Basically to optimize isdefined you need to do ssa construction

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 thought that is a data-flow property. My understanding was that the type/isdefined-ness information for a slot gets reset by NewvarNode, but then both of those get updated along with the abstract interpretation. E.g. within smerge, both typ and undef are being merged.

The purpose of this PR was to eliminate unnecessary :throw_undef_if_not during SSA construction, but I might be making a fundamental mistake.

Copy link
Member

Choose a reason for hiding this comment

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

Turns out Conditional itself is just broken: #55548

function Conditional(slot::Int, @nospecialize(thentype), @nospecialize(elsetype);
isdefined::Bool=false)
assert_nested_slotwrapper(thentype)
assert_nested_slotwrapper(elsetype)
return new(slot, thentype, elsetype)
return new(slot, thentype, elsetype, isdefined)
end
end
Conditional(var::SlotNumber, @nospecialize(thentype), @nospecialize(elsetype)) =
Conditional(slot_id(var), thentype, elsetype)
Conditional(var::SlotNumber, @nospecialize(thentype), @nospecialize(elsetype); isdefined::Bool=false) =
Conditional(slot_id(var), thentype, elsetype; isdefined)

import Core: InterConditional
"""
Expand Down Expand Up @@ -180,6 +185,7 @@ struct StateUpdate
var::SlotNumber
vtype::VarState
conditional::Bool
StateUpdate(var::SlotNumber, vtype::VarState, conditional::Bool=false) = new(var, vtype, conditional)
end

"""
Expand Down Expand Up @@ -305,11 +311,17 @@ end

# `Conditional` and `InterConditional` are valid in opposite contexts
# (i.e. local inference and inter-procedural call), as such they will never be compared
@nospecializeinfer function issubconditional(lattice::AbstractLattice, a::C, b::C) where {C<:AnyConditional}
@nospecializeinfer issubconditional(𝕃::AbstractLattice, a::Conditional, b::Conditional) =
_issubconditional(𝕃, a, b, #=check_isdefined=#true)
@nospecializeinfer issubconditional(𝕃::AbstractLattice, a::InterConditional, b::InterConditional) =
_issubconditional(𝕃, a, b, #=check_isdefined=#false)
@nospecializeinfer function _issubconditional(𝕃::AbstractLattice, a::C, b::C, check_isdefined::Bool) where C<:AnyConditional
if is_same_conditionals(a, b)
if ⊑(lattice, a.thentype, b.thentype)
if ⊑(lattice, a.elsetype, b.elsetype)
return true
if ⊑(𝕃, a.thentype, b.thentype)
if ⊑(𝕃, a.elsetype, b.elsetype)
if !check_isdefined || a.isdefined ≥ b.isdefined
return true
end
end
end
end
Expand Down
12 changes: 12 additions & 0 deletions test/compiler/inference.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6050,6 +6050,18 @@ end |> Core.Compiler.is_nothrow
return nothing
end |> Core.Compiler.is_nothrow

# refine `undef` information from `@isdefined` check
@test Base.infer_effects((Bool,Int)) do c, x
local val
if c
val = x
end
if @isdefined val
return val
end
return zero(Int)
end |> Core.Compiler.is_nothrow

# End to end test case for the partially initialized struct with `PartialStruct`
@noinline broadcast_noescape1(a) = (broadcast(identity, a); nothing)
@test fully_eliminated() do
Expand Down