Remove edges from dead blocks from phi nodes

base/compiler/ssair/driver.jl

@@ -128,8 +128,7 @@ function run_passes(ci::CodeInfo, nargs::Int, sv::OptimizationState)
  #@timeit "verify 2" verify_ir(ir)
  ir = compact!(ir)
  #@Base.show ("before_sroa", ir)
- @timeit "domtree 2" domtree = construct_domtree(ir.cfg)
- @timeit "SROA" ir = getfield_elim_pass!(ir, domtree)
+ @timeit "SROA" ir = getfield_elim_pass!(ir)
  #@Base.show ir.new_nodes
  #@Base.show ("after_sroa", ir)
  ir = adce_pass!(ir)

base/compiler/ssair/ir.jl

@@ -989,7 +989,41 @@ function process_node!(compact::IncrementalCompact, result_idx::Int, inst::Instr
  result[result_idx][:inst] = renumber_ssa2!(stmt, ssa_rename, used_ssas, late_fixup, result_idx, do_rename_ssa)
  result_idx += 1
  elseif isa(stmt, PhiNode)
- values = process_phinode_values(stmt.values, late_fixup, processed_idx, result_idx, ssa_rename, used_ssas, do_rename_ssa)
+ if compact.cfg_transforms_enabled
+ # Rename phi node edges
+ map!(i -> compact.bb_rename_pred[i], stmt.edges, stmt.edges)
+
+ # Remove edges and values associated with dead blocks. Entries in
+ # `values` can be undefined when the phi node refers to something
+ # that is not defined. (This is not a sign that the compiler is
+ # unintentionally leaving some entries in `values` uninitialized.)
+ # For example, consider a reference to a variable that is only
+ # defined if some branch is taken.
+ #
+ # In order to leave undefined values undefined (undefined-ness is
+ # not a value we can copy), we copy only the edges and (defined)
+ # values we want to keep to new arrays initialized with undefined
+ # elements.
+ edges = Vector{Any}(undef, length(stmt.edges))
+ values = Vector{Any}(undef, length(stmt.values))
+ new_index = 1
+ for old_index in 1:length(stmt.edges)
+ if stmt.edges[old_index] != -1
+ edges[new_index] = stmt.edges[old_index]
+ if isassigned(stmt.values, old_index)
+ values[new_index] = stmt.values[old_index]
+ end
+ new_index += 1
+ end
+ end
+ resize!(edges, new_index-1)
+ resize!(values, new_index-1)
+ else
+ edges = stmt.edges
+ values = stmt.values
+ end
+
+ values = process_phinode_values(values, late_fixup, processed_idx, result_idx, ssa_rename, used_ssas, do_rename_ssa)
  # Don't remove the phi node if it is before the definition of its value
  # because doing so can create forward references. This should only
  # happen with dead loops, but can cause problems when optimization
@@ -998,14 +1032,13 @@ function process_node!(compact::IncrementalCompact, result_idx::Int, inst::Instr
  # just to be safe.
  before_def = isassigned(values, 1) && isa(values[1], OldSSAValue) &&
  idx < values[1].id
- if length(stmt.edges) == 1 && isassigned(values, 1) && !before_def &&
+ if length(edges) == 1 && isassigned(values, 1) && !before_def &&
  length(compact.cfg_transforms_enabled ?
  compact.result_bbs[compact.bb_rename_succ[active_bb]].preds :
  compact.ir.cfg.blocks[active_bb].preds) == 1
  # There's only one predecessor left - just replace it
  ssa_rename[idx] = values[1]
  else
- edges = compact.cfg_transforms_enabled ? map!(i->compact.bb_rename_pred[i], stmt.edges, stmt.edges) : stmt.edges
  result[result_idx][:inst] = PhiNode(edges, values)
  result_idx += 1
  end

base/compiler/ssair/passes.jl

@@ -524,7 +524,7 @@ function perform_lifting!(compact::IncrementalCompact,
 end
 
 assertion_counter = 0
-function getfield_elim_pass!(ir::IRCode, domtree::DomTree)
+function getfield_elim_pass!(ir::IRCode)
  compact = IncrementalCompact(ir)
  insertions = Vector{Any}()
  defuses = IdDict{Int, Tuple{IdSet{Int}, SSADefUse}}()
@@ -721,6 +721,13 @@ function getfield_elim_pass!(ir::IRCode, domtree::DomTree)
  used_ssas = copy(compact.used_ssas)
  simple_dce!(compact)
  ir = complete(compact)
+
+ # Compute domtree, needed below, now that we have finished compacting the
+ # IR. This needs to be after we iterate through the IR with
+ # `IncrementalCompact` because removing dead blocks can invalidate the
+ # domtree.
+ @timeit "domtree 2" domtree = construct_domtree(ir.cfg)
+
  # Now go through any mutable structs and see which ones we can eliminate
  for (idx, (intermediaries, defuse)) in defuses
  intermediaries = collect(intermediaries)

test/compiler/irpasses.jl

@@ -180,8 +180,7 @@ let m = Meta.@lower 1 + 1
  src.ssaflags = fill(Int32(0), nstmts)
  ir = Core.Compiler.inflate_ir(src, Any[], Any[Any, Any])
  @test Core.Compiler.verify_ir(ir) === nothing
- domtree = Core.Compiler.construct_domtree(ir.cfg)
- ir = @test_nowarn Core.Compiler.getfield_elim_pass!(ir, domtree)
+ ir = @test_nowarn Core.Compiler.getfield_elim_pass!(ir)
  @test Core.Compiler.verify_ir(ir) === nothing
 end
 

test/compiler/ssair.jl

@@ -3,6 +3,19 @@
 using Base.Meta
 using Core.IR
 const Compiler = Core.Compiler
+using .Compiler: CFG, BasicBlock
+
+make_bb(preds, succs) = BasicBlock(Compiler.StmtRange(0, 0), preds, succs)
+
+function make_ci(code)
+ ci = (Meta.@lower 1 + 1).args[1]
+ ci.code = code
+ nstmts = length(ci.code)
+ ci.ssavaluetypes = nstmts
+ ci.codelocs = fill(Int32(1), nstmts)
+ ci.ssaflags = fill(Int32(0), nstmts)
+ return ci
+end
 
 # TODO: this test is broken
 #let code = Any[
@@ -29,7 +42,6 @@ const Compiler = Core.Compiler
 #end
 
 # Issue #31121
-using .Compiler: CFG, BasicBlock
 
 # We have the following CFG and corresponding DFS numbering:
 #
@@ -48,7 +60,6 @@ using .Compiler: CFG, BasicBlock
 # than `3`, so the idom search missed that `1` is `3`'s semi-dominator). Here
 # we manually construct that CFG and verify that the DFS records the correct
 # parent.
-make_bb(preds, succs) = BasicBlock(Compiler.StmtRange(0, 0), preds, succs)
 let cfg = CFG(BasicBlock[
  make_bb([] , [2, 3]),
  make_bb([1] , [4, 5]),
@@ -136,8 +147,7 @@ end
 @test f32579(0, false) === false
 
 # Test for bug caused by renaming blocks improperly, related to PR #32145
-let ci = (Meta.@lower 1 + 1).args[1]
- ci.code = [
+let ci = make_ci([
  # block 1
  Core.Compiler.GotoIfNot(Expr(:boundscheck), 6),
  # block 2
@@ -155,11 +165,7 @@ let ci = (Meta.@lower 1 + 1).args[1]
  Core.Compiler.ReturnNode(Core.SSAValue(8)),
  # block 6
  Core.Compiler.ReturnNode(Core.SSAValue(8))
- ]
- nstmts = length(ci.code)
- ci.ssavaluetypes = nstmts
- ci.codelocs = fill(Int32(1), nstmts)
- ci.ssaflags = fill(Int32(0), nstmts)
+ ])
  ir = Core.Compiler.inflate_ir(ci)
  ir = Core.Compiler.compact!(ir, true)
  @test Core.Compiler.verify_ir(ir) == nothing
@@ -181,8 +187,7 @@ let ci = (Meta.@lower 1 + 1).args[1]
 end
 
 # Issue #29107
-let ci = (Meta.@lower 1 + 1).args[1]
- ci.code = [
+let ci = make_ci([
  # Block 1
  Core.Compiler.GotoNode(6),
  # Block 2
@@ -197,11 +202,7 @@ let ci = (Meta.@lower 1 + 1).args[1]
  Core.Compiler.GotoNode(2),
  # Block 3
  Core.Compiler.ReturnNode(1000)
- ]
- nstmts = length(ci.code)
- ci.ssavaluetypes = nstmts
- ci.codelocs = fill(Int32(1), nstmts)
- ci.ssaflags = fill(Int32(0), nstmts)
+ ])
  ir = Core.Compiler.inflate_ir(ci)
  ir = Core.Compiler.compact!(ir, true)
  # Make sure that if there is a call to `something` (block 2 should be
@@ -216,3 +217,19 @@ let ci = (Meta.@lower 1 + 1).args[1]
  end
  end
 end
+
+# Make sure dead blocks that are removed are not still referenced in live phi
+# nodes
+let ci = make_ci([
+ # Block 1
+ Core.Compiler.GotoNode(3),
+ # Block 2 (no predecessors)
+ Core.Compiler.ReturnNode(3),
+ # Block 3
+ Core.PhiNode(Any[1, 2], Any[100, 200]),
+ Core.Compiler.ReturnNode(Core.SSAValue(3))
+ ])
+ ir = Core.Compiler.inflate_ir(ci)
+ ir = Core.Compiler.compact!(ir, true)
+ @test Core.Compiler.verify_ir(ir) == nothing
+end