Skip to content

Commit

Permalink
Remove type_lift_pass!/OptimizerLattice (#50257)
Browse files Browse the repository at this point in the history 
This pass (which has become somehwat misnamed) was inserting error checks,
both for undefined slots and undefrefs from sroa'd getfields (at least in
theory - this broke at some point - see #50250). It accomplished this
partly by using the OptimizerLattice, which adjoins the `MaybeUndef` lattice
element to the ordinary incidence lattice. This lattice element indicates
that the SSAValue may potentially have come from an undef slot and
gives it special semantics inside `:isdefined` and `:undefcheck`.

However, in our more recent formalization of lattices, this element is
ill-defined. It is not valid to widen it, because doing so would change
semantics and cause crashes. It would be possible to have a correct version
of this element, but it would require inverting the meaning (i.e. having
all types be maybe-undef and using a NotUndef lattice element). However,
such a change would be expensive and not worth it. This has been causing
me some headaches downstream when trying to use custom lattices and custom
pass pipelines, so I had some extra motivation to do something about it.

This PR just does away with all this complexity. SSA conversion and SROA now
directly insert the requisite `:throw_undef_if_not` checks. This does
increase the size of the IR somewhat earlier in the pipeline, but on
the other hand it saves a full scan over the IR later in the pipeline,
so it's probably a was overall.

While we're here, we fix #50250 by properly inserting the requistie phi
nest inside SROA.
  • Loading branch information
@Keno
Keno committed Jun 22, 2023
1 parent 330c79d commit ec33194
Show file tree
Hide file tree
Showing 18 changed files with 157 additions and 334 deletions.
13 changes: 0 additions & 13 deletions base/compiler/abstractlattice.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,19 +95,6 @@ end
widenlattice(𝕃::InferenceLattice) = 𝕃.parent
is_valid_lattice_norec(::InferenceLattice, @nospecialize(elem)) = isa(elem, LimitedAccuracy)

"""
struct OptimizerLattice{𝕃<:AbstractLattice} <: AbstractLattice
The lattice used by the optimizer.
Extends a base lattice `𝕃` and adjoins `MaybeUndef`.
"""
struct OptimizerLattice{𝕃<:AbstractLattice} <: AbstractLattice
parent::𝕃
end
OptimizerLattice() = OptimizerLattice(SimpleInferenceLattice.instance)
widenlattice(𝕃::OptimizerLattice) = 𝕃.parent
is_valid_lattice_norec(::OptimizerLattice, @nospecialize(elem)) = isa(elem, MaybeUndef)

"""
tmeet(𝕃::AbstractLattice, a, b::Type)
Expand Down
1 change: 0 additions & 1 deletion base/compiler/optimize.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,6 @@ function run_passes(
@pass "compact 2" ir = compact!(ir)
@pass "SROA" ir = sroa_pass!(ir, sv.inlining)
@pass "ADCE" ir = adce_pass!(ir, sv.inlining)
@pass "type lift" ir = type_lift_pass!(ir)
@pass "compact 3" ir = compact!(ir)
if JLOptions().debug_level == 2
@timeit "verify 3" (verify_ir(ir); verify_linetable(ir.linetable))
Expand Down
5 changes: 2 additions & 3 deletions base/compiler/ssair/EscapeAnalysis/EscapeAnalysis.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import ._TOP_MOD: # Base definitions
pop!, push!, pushfirst!, empty!, delete!, max, min, enumerate, unwrap_unionall,
ismutabletype
import Core.Compiler: # Core.Compiler specific definitions
Bottom, OptimizerLattice, InferenceResult, IRCode, IR_FLAG_NOTHROW,
Bottom, InferenceResult, IRCode, IR_FLAG_NOTHROW, SimpleInferenceLattice,
isbitstype, isexpr, is_meta_expr_head, println, widenconst, argextype, singleton_type,
fieldcount_noerror, try_compute_field, try_compute_fieldidx, hasintersect, ⊑,
intrinsic_nothrow, array_builtin_common_typecheck, arrayset_typecheck,
Expand All @@ -42,7 +42,7 @@ end

const AInfo = IdSet{Any}
const LivenessSet = BitSet
const 𝕃ₒ = OptimizerLattice()
const 𝕃ₒ = SimpleInferenceLattice.instance

"""
x::EscapeInfo
Expand Down Expand Up @@ -707,7 +707,6 @@ function analyze_escapes(ir::IRCode, nargs::Int, call_resolved::Bool, get_escape
continue
elseif head === :static_parameter || # this exists statically, not interested in its escape
head === :copyast || # XXX can this account for some escapes?
head === :undefcheck || # XXX can this account for some escapes?
head === :isdefined || # just returns `Bool`, nothing accounts for any escapes
head === :gc_preserve_begin || # `GC.@preserve` expressions themselves won't be used anywhere
head === :gc_preserve_end # `GC.@preserve` expressions themselves won't be used anywhere
Expand Down
2 changes: 1 addition & 1 deletion base/compiler/ssair/inlining.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,7 @@ function fix_va_argexprs!(insert_node!::Inserter, inline_target::Union{IRCode, I
push!(tuple_call.args, arg)
push!(tuple_typs, argextype(arg, inline_target))
end
tuple_typ = tuple_tfunc(OptimizerLattice(), tuple_typs)
tuple_typ = tuple_tfunc(SimpleInferenceLattice.instance, tuple_typs)
tuple_inst = NewInstruction(tuple_call, tuple_typ, line_idx)
push!(newargexprs, insert_node!(tuple_inst))
return newargexprs
Expand Down
2 changes: 1 addition & 1 deletion base/compiler/ssair/ir.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ function is_relevant_expr(e::Expr)
:new, :splatnew, :(=), :(&),
:gc_preserve_begin, :gc_preserve_end,
:foreigncall, :isdefined, :copyast,
:undefcheck, :throw_undef_if_not,
:throw_undef_if_not,
:cfunction, :method, :pop_exception,
:new_opaque_closure)
end
Expand Down
188 changes: 25 additions & 163 deletions base/compiler/ssair/passes.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ struct LiftedValue
LiftedValue(@nospecialize val) = new(val)
end
const LiftedLeaves = IdDict{Any, Union{Nothing,LiftedValue}}
const LiftedDefs = IdDict{Any, Bool}

# try to compute lifted values that can replace `getfield(x, field)` call
# where `x` is an immutable struct that are defined at any of `leaves`
Expand Down Expand Up @@ -505,8 +506,6 @@ function walk_to_def(compact::IncrementalCompact, @nospecialize(leaf))
return Pair{Any, Any}(def, leaf)
end

make_MaybeUndef(@nospecialize(typ)) = isa(typ, MaybeUndef) ? typ : MaybeUndef(typ)

"""
lift_comparison!(cmp, compact::IncrementalCompact, idx::Int, stmt::Expr, 𝕃ₒ::AbstractLattice)
Expand Down Expand Up @@ -620,7 +619,7 @@ end
struct SkipToken end; const SKIP_TOKEN = SkipToken()

function lifted_value(compact::IncrementalCompact, @nospecialize(old_node_ssa#=::AnySSAValue=#), @nospecialize(old_value),
lifted_philikes::Vector{LiftedPhilike}, lifted_leaves::LiftedLeaves, reverse_mapping::IdDict{AnySSAValue, Int})
lifted_philikes::Vector{LiftedPhilike}, lifted_leaves::Union{LiftedLeaves, LiftedDefs}, reverse_mapping::IdDict{AnySSAValue, Int})
val = old_value
if is_old(compact, old_node_ssa) && isa(val, SSAValue)
val = OldSSAValue(val.id)
Expand All @@ -630,6 +629,9 @@ function lifted_value(compact::IncrementalCompact, @nospecialize(old_node_ssa#=:
end
if val in keys(lifted_leaves)
lifted_val = lifted_leaves[val]
if isa(lifted_leaves, LiftedDefs)
return lifted_val
end
lifted_val === nothing && return UNDEF_TOKEN
val = lifted_val.val
if isa(val, AnySSAValue)
Expand All @@ -653,7 +655,7 @@ end
function perform_lifting!(compact::IncrementalCompact,
visited_philikes::Vector{AnySSAValue}, @nospecialize(cache_key),
lifting_cache::IdDict{Pair{AnySSAValue, Any}, AnySSAValue},
@nospecialize(result_t), lifted_leaves::LiftedLeaves, @nospecialize(stmt_val),
@nospecialize(result_t), lifted_leaves::Union{LiftedLeaves, LiftedDefs}, @nospecialize(stmt_val),
lazydomtree::Union{LazyDomtree,Nothing})
reverse_mapping = IdDict{AnySSAValue, Int}()
for id in 1:length(visited_philikes)
Expand Down Expand Up @@ -912,10 +914,11 @@ In a case when all usages are fully eliminated, `struct` allocation may also be
a result of succeeding dead code elimination.
"""
function sroa_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing)
𝕃ₒ = inlining === nothing ? OptimizerLattice() : optimizer_lattice(inlining.interp)
𝕃ₒ = inlining === nothing ? SimpleInferenceLattice.instance : optimizer_lattice(inlining.interp)
compact = IncrementalCompact(ir)
defuses = nothing # will be initialized once we encounter mutability in order to reduce dynamic allocations
lifting_cache = IdDict{Pair{AnySSAValue, Any}, AnySSAValue}()
def_lifting_cache = IdDict{Pair{AnySSAValue, Any}, AnySSAValue}()
# initialization of domtree is delayed to avoid the expensive computation in many cases
lazydomtree = LazyDomtree(ir)
for ((_, idx), stmt) in compact
Expand Down Expand Up @@ -1082,23 +1085,30 @@ function sroa_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing)
lifted_result === nothing && continue
lifted_leaves, any_undef = lifted_result

if any_undef
result_t = make_MaybeUndef(result_t)
end

val = perform_lifting!(compact,
lifted_val = perform_lifting!(compact,
visited_philikes, field, lifting_cache, result_t, lifted_leaves, val, lazydomtree)

# Insert the undef check if necessary
if any_undef && val === nothing
if any_undef
if lifted_val === nothing
def_val = false
else
lifted_leaves_def = LiftedDefs()
for (k, v) in pairs(lifted_leaves)
lifted_leaves_def[k] = v === nothing ? false : true
end
def_val = perform_lifting!(compact,
visited_philikes, field, def_lifting_cache, result_t, lifted_leaves_def, val, lazydomtree).val
end
insert_node!(compact, SSAValue(idx), non_effect_free(NewInstruction(
Expr(:throw_undef_if_not, Symbol("##getfield##"), false), Nothing)))
Expr(:throw_undef_if_not, Symbol("##getfield##"), def_val), Nothing)))

else
# val must be defined
@assert val !== nothing
@assert lifted_val !== nothing
end

compact[idx] = val === nothing ? nothing : val.val
compact[idx] = lifted_val === nothing ? nothing : lifted_val.val
compact[SSAValue(idx)][:flag] |= IR_FLAG_REFINED
end

Expand Down Expand Up @@ -1572,7 +1582,6 @@ function mark_phi_cycles!(compact::IncrementalCompact, safe_phis::SPCSet, phi::I
end

function is_some_union(@nospecialize(t))
isa(t, MaybeUndef) && (t = t.typ)
return isa(t, Union)
end

Expand Down Expand Up @@ -1626,7 +1635,7 @@ the `typeassert` elimination depends on the transformation by `canonicalize_type
within `sroa_pass!` which redirects references of `typeassert`ed value to the corresponding `PiNode`.
"""
function adce_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing)
𝕃ₒ = inlining === nothing ? OptimizerLattice() : optimizer_lattice(inlining.interp)
𝕃ₒ = inlining === nothing ? SimpleInferenceLattice.instance : optimizer_lattice(inlining.interp)
phi_uses = fill(0, length(ir.stmts) + length(ir.new_nodes))
all_phis = Int[]
unionphis = Pair{Int,Any}[] # sorted
Expand Down Expand Up @@ -1751,153 +1760,6 @@ function adce_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing)
return complete(compact)
end

function type_lift_pass!(ir::IRCode)
lifted_undef = IdDict{Int, Any}()
insts = ir.stmts
for idx in 1:length(insts)
stmt = insts[idx][:inst]
stmt isa Expr || continue
if (stmt.head === :isdefined || stmt.head === :undefcheck)
# after optimization, undef can only show up by being introduced in
# a phi node (or an UpsilonNode() argument to a PhiC node), so lift
# all these nodes that have maybe undef values
val = stmt.args[(stmt.head === :isdefined) ? 1 : 2]
if stmt.head === :isdefined && (val isa GlobalRef || isexpr(val, :static_parameter) ||
val isa Argument || val isa Symbol)
# this is a legal node, so assume it was not introduced by
# slot2ssa (at worst, we might leave in a runtime check that
# shouldn't have been there)
continue
end
# otherwise, we definitely have a corrupt node from slot2ssa, and
# must fix or delete that now
processed = IdDict{Int, Union{SSAValue, Bool}}()
def = val
while true
# peek through PiNodes
isa(val, SSAValue) || break
def = insts[val.id][:inst]
isa(def, PiNode) || break
val = def.val
end
if !isa(val, SSAValue) || (!isa(def, PhiNode) && !isa(def, PhiCNode))
# in most cases, reaching this statement implies we had a value
if stmt.head === :undefcheck
insts[idx][:inst] = nothing
else
insts[idx][:inst] = true
end
continue
end
stmt_id = val.id
worklist = Tuple{Int, Int, SSAValue, Int}[(stmt_id, 0, SSAValue(0), 0)]
if !haskey(lifted_undef, stmt_id)
first = true
while !isempty(worklist)
item, w_up_id, which, use = pop!(worklist)
def = insts[item][:inst]
if isa(def, PhiNode)
edges = copy(def.edges)
values = Vector{Any}(undef, length(edges))
new_phi = if length(values) == 0
false
else
insert_node!(ir, item, NewInstruction(PhiNode(edges, values), Bool))
end
else
def = def::PhiCNode
values = Vector{Any}(undef, length(def.values))
new_phi = if length(values) == 0
false
else
insert_node!(ir, item, NewInstruction(PhiCNode(values), Bool))
end
end
processed[item] = new_phi
if first
lifted_undef[stmt_id] = new_phi
first = false
end
local id::Int = 0
all_same = true
local last_val
for i = 1:length(values)
if !isassigned(def.values, i)
val = false
elseif !isa(def.values[i], SSAValue)
val = true
else
up_id = id = (def.values[i]::SSAValue).id
@label restart
if !isa(ir.stmts[id][:type], MaybeUndef)
val = true
else
node = insts[id][:inst]
if isa(node, UpsilonNode)
if !isdefined(node, :val)
val = false
elseif !isa(node.val, SSAValue)
val = true
else
id = (node.val::SSAValue).id
@goto restart
end
else
while isa(node, PiNode)
id = (node.val::SSAValue).id
node = insts[id][:inst]
end
if isa(node, Union{PhiNode, PhiCNode})
if haskey(processed, id)
val = processed[id]
else
# TODO: Re-check after convergence whether all the values are the same
all_same = false
push!(worklist, (id, up_id, new_phi::SSAValue, i))
continue
end
else
val = true
end
end
end
end
if isa(def, PhiNode)
if !@isdefined(last_val)
last_val = val
elseif all_same
all_same &= last_val === val
end
values[i] = val
else
values[i] = insert_node!(ir, up_id, NewInstruction(UpsilonNode(val), Bool))
end
end
if all_same && @isdefined(last_val)
# Decay the PhiNode back to the single value
ir[new_phi][:inst] = last_val
isa(last_val, Bool) && (processed[item] = last_val)
end
if which !== SSAValue(0)
phi = ir[which][:inst]
if isa(phi, PhiNode)
phi.values[use] = new_phi
elseif isa(phi, PhiCNode)
phi.values[use] = insert_node!(ir, w_up_id, NewInstruction(UpsilonNode(new_phi), Bool))
end
end
end
end
inst = lifted_undef[stmt_id]
if stmt.head === :undefcheck
inst = Expr(:throw_undef_if_not, stmt.args[1], inst)
end
insts[idx][:inst] = inst
end
end
ir
end

function is_bb_empty(ir::IRCode, bb::BasicBlock)
isempty(bb.stmts) && return true
if length(bb.stmts) == 1
Expand Down
Loading

0 comments on commit ec33194

Please sign in to comment.