diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl index 326a4094e4134..3792027f82aa4 100644 --- a/base/compiler/ssair/inlining.jl +++ b/base/compiler/ssair/inlining.jl @@ -1918,7 +1918,7 @@ function ssa_substitute_op!(insert_node!::Inserter, subst_inst::Instruction, @no end end end - isa(val, Union{SSAValue, NewSSAValue}) && return val # avoid infinite loop + isa(val, AnySSAValue) && return val # avoid infinite loop urs = userefs(val) for op in urs op[] = ssa_substitute_op!(insert_node!, subst_inst, op[], ssa_substitute) diff --git a/base/compiler/ssair/ir.jl b/base/compiler/ssair/ir.jl index dc19c9a09fbba..fbb17074950c2 100644 --- a/base/compiler/ssair/ir.jl +++ b/base/compiler/ssair/ir.jl @@ -464,7 +464,7 @@ struct UndefToken end; const UNDEF_TOKEN = UndefToken() isdefined(stmt, :val) || return OOB_TOKEN op == 1 || return OOB_TOKEN return stmt.val - elseif isa(stmt, Union{SSAValue, NewSSAValue, GlobalRef}) + elseif isa(stmt, Union{AnySSAValue, GlobalRef}) op == 1 || return OOB_TOKEN return stmt elseif isa(stmt, UpsilonNode) @@ -520,7 +520,7 @@ end elseif isa(stmt, ReturnNode) op == 1 || throw(BoundsError()) stmt = typeof(stmt)(v) - elseif isa(stmt, Union{SSAValue, NewSSAValue, GlobalRef}) + elseif isa(stmt, Union{AnySSAValue, GlobalRef}) op == 1 || throw(BoundsError()) stmt = v elseif isa(stmt, UpsilonNode) @@ -550,7 +550,7 @@ end function userefs(@nospecialize(x)) relevant = (isa(x, Expr) && is_relevant_expr(x)) || - isa(x, GotoIfNot) || isa(x, ReturnNode) || isa(x, SSAValue) || isa(x, NewSSAValue) || + isa(x, GotoIfNot) || isa(x, ReturnNode) || isa(x, SSAValue) || isa(x, OldSSAValue) || isa(x, NewSSAValue) || isa(x, PiNode) || isa(x, PhiNode) || isa(x, PhiCNode) || isa(x, UpsilonNode) || isa(x, EnterNode) return UseRefIterator(x, relevant) end @@ -781,6 +781,16 @@ function dominates_ssa(compact::IncrementalCompact, domtree::DomTree, x::AnySSAV xb = block_for_inst(compact, x) yb = block_for_inst(compact, y) if xb == yb + if isa(compact[x][:stmt], PhiNode) + if isa(compact[y][:stmt], PhiNode) + # A node dominates another only if it dominates all uses of that note. + # Usually that is not a distinction. However, for phi nodes, the use + # occurs on the edge to the predecessor block. Thus, by definition, for + # any other PhiNode in the same BB there must be (at least) one edge + # that this phi node does not dominate. + return false + end + end xinfo = yinfo = nothing if isa(x, OldSSAValue) x′ = compact.ssa_rename[x.id]::SSAValue @@ -939,17 +949,23 @@ function insert_node!(compact::IncrementalCompact, @nospecialize(before), newins end end +function maybe_reopen_bb!(compact) + result_idx = compact.result_idx + result_bbs = compact.cfg_transform.result_bbs + if (compact.active_result_bb == length(result_bbs) + 1) || + result_idx == first(result_bbs[compact.active_result_bb].stmts) + compact.active_result_bb -= 1 + return true + end + return false +end + function insert_node_here!(compact::IncrementalCompact, newinst::NewInstruction, reverse_affinity::Bool=false) newline = newinst.line::Int32 refinish = false result_idx = compact.result_idx result_bbs = compact.cfg_transform.result_bbs - if reverse_affinity && - ((compact.active_result_bb == length(result_bbs) + 1) || - result_idx == first(result_bbs[compact.active_result_bb].stmts)) - compact.active_result_bb -= 1 - refinish = true - end + refinish = reverse_affinity && maybe_reopen_bb!(compact) if result_idx > length(compact.result) @assert result_idx == length(compact.result) + 1 resize!(compact, result_idx) @@ -964,10 +980,17 @@ function insert_node_here!(compact::IncrementalCompact, newinst::NewInstruction, end function delete_inst_here!(compact::IncrementalCompact) + # If we already closed this bb, reopen it for our modification + refinish = maybe_reopen_bb!(compact) + # Delete the statement, update refcounts etc compact[SSAValue(compact.result_idx-1)] = nothing + # Pretend that we never compacted this statement in the first place compact.result_idx -= 1 + + refinish && finish_current_bb!(compact, 0) + return nothing end @@ -1169,13 +1192,15 @@ function process_phinode_values(old_values::Vector{Any}, late_fixup::Vector{Int} end elseif isa(val, NewSSAValue) if val.id < 0 - push!(late_fixup, result_idx) new_new_used_ssas[-val.id] += 1 else @assert do_rename_ssa val = SSAValue(val.id) end end + if isa(val, NewSSAValue) + push!(late_fixup, result_idx) + end values[i] = val end return values @@ -1196,6 +1221,9 @@ function renumber_ssa2(val::SSAValue, ssanums::Vector{Any}, used_ssas::Vector{In end if isa(val, SSAValue) used_ssas[val.id] += 1 + elseif isa(val, NewSSAValue) + @assert val.id < 0 + new_new_used_ssas[-val.id] += 1 end return val end @@ -1834,14 +1862,20 @@ function fixup_node(compact::IncrementalCompact, @nospecialize(stmt), reify_new_ return FixedNode(stmt, true) end elseif isa(stmt, OldSSAValue) - val = compact.ssa_rename[stmt.id] - if isa(val, Refined) - val = val.val + node = compact.ssa_rename[stmt.id] + if isa(node, Refined) + node = node.val end - if isa(val, SSAValue) - compact.used_ssas[val.id] += 1 + needs_fixup = false + if isa(node, NewSSAValue) + (;node, needs_fixup) = fixup_node(compact, node, reify_new_nodes) + end + if isa(node, SSAValue) + compact.used_ssas[node.id] += 1 + elseif isa(node, NewSSAValue) + compact.new_new_used_ssas[-node.id] += 1 end - return FixedNode(val, false) + return FixedNode(node, needs_fixup) else urs = userefs(stmt) fixup = false diff --git a/base/compiler/ssair/passes.jl b/base/compiler/ssair/passes.jl index 944e444bf6d06..424ab23b5a28a 100644 --- a/base/compiler/ssair/passes.jl +++ b/base/compiler/ssair/passes.jl @@ -473,9 +473,14 @@ function lift_arg!( if is_old(compact, leaf) && isa(lifted, SSAValue) lifted = OldSSAValue(lifted.id) if already_inserted(compact, lifted) - lifted = compact.ssa_rename[lifted.id] - if isa(lifted, Refined) - lifted = lifted.val + new_lifted = compact.ssa_rename[lifted.id] + if isa(new_lifted, Refined) + new_lifted = new_lifted.val + end + # Special case: If lifted happens to be the statement we're currently processing, + # leave it as old SSAValue in case we decide to handle this in the renamer + if !isa(new_lifted, SSAValue) || new_lifted != SSAValue(compact.result_idx-1) + lifted = new_lifted end end end @@ -602,10 +607,12 @@ function lift_comparison_leaves!(@specialize(tfunc), end # perform lifting - lifted_val = perform_lifting!(compact, - visited_philikes, cmp, Bool, lifted_leaves::LiftedLeaves, val, nothing)::LiftedValue + (lifted_val, nest) = perform_lifting!(compact, + visited_philikes, cmp, Bool, lifted_leaves::LiftedLeaves, val, nothing) + + compact[idx] = (lifted_val::LiftedValue).val - compact[idx] = lifted_val.val + finish_phi_nest!(compact, nest) end struct IfElseCall @@ -651,80 +658,22 @@ function lifted_value(compact::IncrementalCompact, @nospecialize(old_node_ssa#=: end function is_old(compact, @nospecialize(old_node_ssa)) - isa(old_node_ssa, OldSSAValue) && - !is_pending(compact, old_node_ssa) && - !already_inserted(compact, old_node_ssa) + isa(old_node_ssa, OldSSAValue) || return false + is_pending(compact, old_node_ssa) && return false + already_inserted(compact, old_node_ssa) && return false + return true end -function perform_lifting!(compact::IncrementalCompact, - visited_philikes::Vector{AnySSAValue}, @nospecialize(cache_key), - @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) - reverse_mapping[visited_philikes[id]] = id - end - - # Check if all the lifted leaves are the same - local the_leaf - all_same = true - for (_, val) in lifted_leaves - if !@isdefined(the_leaf) - the_leaf = val - continue - end - if val !== the_leaf - all_same = false - end - end - - the_leaf_val = isa(the_leaf, LiftedValue) ? the_leaf.val : nothing - if !isa(the_leaf_val, SSAValue) - all_same = false - end - - if all_same - dominates_all = true - if lazydomtree !== nothing - domtree = get!(lazydomtree) - for item in visited_philikes - if !dominates_ssa(compact, domtree, the_leaf_val, item) - dominates_all = false - break - end - end - if dominates_all - return the_leaf - end - end - end - - # Insert PhiNodes - nphilikes = length(visited_philikes) - lifted_philikes = Vector{LiftedPhilike}(undef, nphilikes) - for i = 1:nphilikes - old_ssa = visited_philikes[i] - old_inst = compact[old_ssa] - old_node = old_inst[:stmt]::Union{PhiNode,Expr} - if isa(old_node, PhiNode) - new_node = PhiNode() - ssa = insert_node!(compact, old_ssa, effect_free_and_nothrow(NewInstruction(new_node, result_t))) - lifted_philikes[i] = LiftedPhilike(ssa, new_node, true) - else - @assert is_known_call(old_node, Core.ifelse, compact) - ifelse_func, condition = old_node.args - if is_old(compact, old_ssa) && isa(condition, SSAValue) - condition = OldSSAValue(condition.id) - end - - new_node = Expr(:call, ifelse_func, condition) # Renamed then_result, else_result added below - new_inst = NewInstruction(new_node, result_t, NoCallInfo(), old_inst[:line], old_inst[:flag]) - - ssa = insert_node!(compact, old_ssa, new_inst, #= attach_after =# true) - lifted_philikes[i] = LiftedPhilike(ssa, IfElseCall(new_node), true) - end - end +struct PhiNest + visited_philikes::Vector{AnySSAValue} + lifted_philikes::Vector{LiftedPhilike} + lifted_leaves::Union{LiftedLeaves, LiftedDefs} + reverse_mapping::IdDict{AnySSAValue, Int} +end +function finish_phi_nest!(compact::IncrementalCompact, nest::PhiNest) + (;visited_philikes, lifted_philikes, lifted_leaves, reverse_mapping) = nest + nphilikes = length(lifted_philikes) # Fix up arguments for i = 1:nphilikes (old_node_ssa, lf) = visited_philikes[i], lifted_philikes[i] @@ -782,6 +731,91 @@ function perform_lifting!(compact::IncrementalCompact, push!(lfnode.call.args, else_result) end end +end + +function perform_lifting!(compact::IncrementalCompact, + visited_philikes::Vector{AnySSAValue}, @nospecialize(cache_key), + @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) + reverse_mapping[visited_philikes[id]] = id + end + + # Check if all the lifted leaves are the same + local the_leaf + all_same = true + for (_, val) in lifted_leaves + if !@isdefined(the_leaf) + the_leaf = val + continue + end + if val !== the_leaf + all_same = false + end + end + + if all_same && isa(the_leaf, LiftedValue) + dominates_all = true + the_leaf_val = the_leaf.val + if isa(the_leaf_val, AnySSAValue) + if lazydomtree === nothing + # Must conservatively assume this + dominates_all = false + else + # This code guards against the possibility of accidentally forwarding a value from a + # previous iteration. Consider for example: + # + # %p = phi(%arg, %t) + # %b = <...> + # %c = getfield(%p, 1) + # %t = tuple(%b) + # + # It would be incorrect to replace `%c` by `%b`, because that would read the value of + # `%b` in the *current* iteration, while the value of `%b` that comes in via `%p` is + # that of the previous iteration. + domtree = get!(lazydomtree) + for item in visited_philikes + if !dominates_ssa(compact, domtree, the_leaf_val, item) + dominates_all = false + break + end + end + end + end + if dominates_all + if isa(the_leaf, OldSSAValue) + the_leaf = simple_walk(compact, the_leaf) + end + return Pair{Any, PhiNest}(the_leaf, PhiNest(visited_philikes, Vector{LiftedPhilike}(undef, 0), lifted_leaves, reverse_mapping)) + end + end + + # Insert PhiNodes + nphilikes = length(visited_philikes) + lifted_philikes = Vector{LiftedPhilike}(undef, nphilikes) + for i = 1:nphilikes + old_ssa = visited_philikes[i] + old_inst = compact[old_ssa] + old_node = old_inst[:stmt]::Union{PhiNode,Expr} + if isa(old_node, PhiNode) + new_node = PhiNode() + ssa = insert_node!(compact, old_ssa, effect_free_and_nothrow(NewInstruction(new_node, result_t))) + lifted_philikes[i] = LiftedPhilike(ssa, new_node, true) + else + @assert is_known_call(old_node, Core.ifelse, compact) + ifelse_func, condition = old_node.args + if is_old(compact, old_ssa) && isa(condition, SSAValue) + condition = OldSSAValue(condition.id) + end + + new_node = Expr(:call, ifelse_func, condition) # Renamed then_result, else_result added below + new_inst = NewInstruction(new_node, result_t, NoCallInfo(), old_inst[:line], old_inst[:flag]) + + ssa = insert_node!(compact, old_ssa, new_inst, #= attach_after =# true) + lifted_philikes[i] = LiftedPhilike(ssa, IfElseCall(new_node), true) + end + end # Fixup the stmt itself if isa(stmt_val, Union{SSAValue, OldSSAValue}) @@ -789,12 +823,12 @@ function perform_lifting!(compact::IncrementalCompact, end if stmt_val in keys(lifted_leaves) - return lifted_leaves[stmt_val] + stmt_val = lifted_leaves[stmt_val] elseif isa(stmt_val, AnySSAValue) && stmt_val in keys(reverse_mapping) - return LiftedValue(lifted_philikes[reverse_mapping[stmt_val]].ssa) + stmt_val = LiftedValue(lifted_philikes[reverse_mapping[stmt_val]].ssa) end - return stmt_val # N.B. should never happen + return Pair{Any, PhiNest}(stmt_val, PhiNest(visited_philikes, lifted_philikes, lifted_leaves, reverse_mapping)) end function lift_svec_ref!(compact::IncrementalCompact, idx::Int, stmt::Expr) @@ -882,10 +916,11 @@ function lift_keyvalue_get!(compact::IncrementalCompact, idx::Int, stmt::Expr, result_t = tmerge(𝕃ₒ, result_t, argextype(v.val, compact)) end - lifted_val = perform_lifting!(compact, + (lifted_val, nest) = perform_lifting!(compact, visited_philikes, key, result_t, lifted_leaves, collection, nothing) compact[idx] = lifted_val === nothing ? nothing : Expr(:call, Core.tuple, lifted_val.val) + finish_phi_nest!(compact, nest) if lifted_val !== nothing if !⊑(𝕃ₒ, compact[SSAValue(idx)][:type], result_t) compact[SSAValue(idx)][:flag] |= IR_FLAG_REFINED @@ -1077,7 +1112,7 @@ function sroa_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing) defuses = nothing # will be initialized once we encounter mutability in order to reduce dynamic allocations # initialization of domtree is delayed to avoid the expensive computation in many cases lazydomtree = LazyDomtree(ir) - for ((_, idx), stmt) in compact + for ((old_idx, idx), stmt) in compact # check whether this statement is `getfield` / `setfield!` (or other "interesting" statement) isa(stmt, Expr) || continue is_setfield = is_isdefined = is_finalizer = is_keyvalue_get = false @@ -1248,9 +1283,27 @@ function sroa_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing) result_t = tmerge(𝕃ₒ, result_t, argextype(v.val, compact)) end - lifted_val = perform_lifting!(compact, + (lifted_val, nest) = perform_lifting!(compact, visited_philikes, field, result_t, lifted_leaves, val, lazydomtree) + node_was_deleted = false + line = compact[SSAValue(idx)][:line] + if lifted_val !== nothing && !⊑(𝕃ₒ, compact[SSAValue(idx)][:type], result_t) + compact[idx] = lifted_val === nothing ? nothing : lifted_val.val + add_flag!(compact[SSAValue(idx)], IR_FLAG_REFINED) + elseif lifted_val === nothing || isa(lifted_val.val, AnySSAValue) + # Save some work in a later compaction, by inserting this into the renamer now, + # but only do this if we didn't set the REFINED flag, to save work for irinterp + # in revisiting only the renamings that came through *this* idx. + delete_inst_here!(compact) + compact.ssa_rename[old_idx] = lifted_val === nothing ? nothing : lifted_val.val + node_was_deleted = true + else + compact[idx] = lifted_val === nothing ? nothing : lifted_val.val + end + + finish_phi_nest!(compact, nest) + # Insert the undef check if necessary if any_undef if lifted_val === nothing @@ -1260,23 +1313,22 @@ function sroa_pass!(ir::IRCode, inlining::Union{Nothing,InliningState}=nothing) for (k, v) in pairs(lifted_leaves) lifted_leaves_def[k] = v === nothing ? false : true end - def_val = perform_lifting!(compact, - visited_philikes, field, Bool, lifted_leaves_def, val, lazydomtree).val + (def_val, nest) = perform_lifting!(compact, + visited_philikes, field, Bool, lifted_leaves_def, val, lazydomtree) + def_val = (def_val::LiftedValue).val + finish_phi_nest!(compact, nest) + end + ni = NewInstruction( + Expr(:throw_undef_if_not, Symbol("##getfield##"), def_val), Nothing, line) + if node_was_deleted + insert_node_here!(compact, ni, true) + else + insert_node!(compact, SSAValue(idx), ni) end - insert_node!(compact, SSAValue(idx), NewInstruction( - Expr(:throw_undef_if_not, Symbol("##getfield##"), def_val), Nothing)) - else # val must be defined @assert lifted_val !== nothing end - - compact[idx] = lifted_val === nothing ? nothing : lifted_val.val - if lifted_val !== nothing - if !⊑(𝕃ₒ, compact[SSAValue(idx)][:type], result_t) - add_flag!(compact[SSAValue(idx)], IR_FLAG_REFINED) - end - end end non_dce_finish!(compact) diff --git a/test/compiler/irpasses.jl b/test/compiler/irpasses.jl index 8dbae2dca9649..17bc9a5beef52 100644 --- a/test/compiler/irpasses.jl +++ b/test/compiler/irpasses.jl @@ -1615,3 +1615,42 @@ let code = Any[ Core.Compiler.__set_check_ssa_counts(false) end end + +# Test SROA all_same on NewNode +let code = Any[ + # Block 1 + Expr(:call, tuple, Argument(1)), + GotoIfNot(Argument(4), 5), + # Block 2 + Expr(:call, tuple, Argument(2)), + GotoIfNot(Argument(4), 9), + # Block 3 + PhiNode(Int32[2, 4], Any[SSAValue(1), SSAValue(3)]), + Expr(:call, getfield, SSAValue(5), 1), + Expr(:call, tuple, SSAValue(6), Argument(2)), # ::Tuple{Int, Int} + Expr(:call, tuple, SSAValue(7), Argument(3)), # ::Tuple{Tuple{Int, Int}, Int} + # Block 4 + PhiNode(Int32[4, 8], Any[nothing, SSAValue(8)]), + Expr(:call, Core.Intrinsics.not_int, Argument(4)), + GotoIfNot(SSAValue(10), 13), + # Block 5 + ReturnNode(1), + # Block 6 + PiNode(SSAValue(9), Tuple{Tuple{Int, Int}, Int}), + Expr(:call, getfield, SSAValue(13), 1), + Expr(:call, getfield, SSAValue(14), 1), + ReturnNode(SSAValue(15)) +] + + argtypes = Any[Int, Int, Int, Bool] + ssavaluetypes = Any[Tuple{Int}, Any, Tuple{Int}, Any, Tuple{Int}, Int, Tuple{Int, Int}, Tuple{Tuple{Int, Int}, Int}, + Union{Nothing, Tuple{Tuple{Int, Int}, Int}}, Bool, Any, Any, + Tuple{Tuple{Int, Int}, Int}, + Tuple{Int, Int}, Int, Any] + ir = make_ircode(code; slottypes=argtypes, ssavaluetypes) + Core.Compiler.verify_ir(ir) + ir = Core.Compiler.sroa_pass!(ir) + Core.Compiler.verify_ir(ir) + ir = Core.Compiler.compact!(ir) + Core.Compiler.verify_ir(ir) +end