Skip to content

Commit

Permalink
Also update phi counts in adce pass (#47181)
Browse files Browse the repository at this point in the history
The whole point of this pass is to compute and compare the
counts of all SSA value uses vs those of only-phi uses to
find SSA values that have no real uses. In #47080, I updated
the code to properly account for removal of phi edges in
the SSA count, but neglected to do the same in the phi-only
count, leading to #47180. Fix that.

Fixes #47180
  • Loading branch information
Keno committed Oct 17, 2022
1 parent 392bc97 commit c2dfe1d
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 8 deletions.
4 changes: 3 additions & 1 deletion base/compiler/ssair/ir.jl
Original file line number Diff line number Diff line change
Expand Up @@ -950,6 +950,8 @@ end
__set_check_ssa_counts(onoff::Bool) = __check_ssa_counts__[] = onoff
const __check_ssa_counts__ = fill(false)

should_check_ssa_counts() = __check_ssa_counts__[]

function _oracle_check(compact::IncrementalCompact)
observed_used_ssas = Core.Compiler.find_ssavalue_uses1(compact)
for i = 1:length(observed_used_ssas)
Expand Down Expand Up @@ -1683,7 +1685,7 @@ end
function complete(compact::IncrementalCompact)
result_bbs = resize!(compact.result_bbs, compact.active_result_bb-1)
cfg = CFG(result_bbs, Int[first(result_bbs[i].stmts) for i in 2:length(result_bbs)])
if __check_ssa_counts__[]
if should_check_ssa_counts()
oracle_check(compact)
end

Expand Down
34 changes: 27 additions & 7 deletions base/compiler/ssair/passes.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1511,6 +1511,30 @@ function is_union_phi(compact::IncrementalCompact, idx::Int)
return is_some_union(inst[:type])
end

function kill_phi!(compact::IncrementalCompact, phi_uses::Vector{Int},
to_drop::Union{Vector{Int}, UnitRange{Int}},
ssa::SSAValue, phi::PhiNode, delete_inst::Bool = false)
for d in to_drop
if isassigned(phi.values, d)
val = phi.values[d]
if !delete_inst
# Deleting the inst will update compact's use count, so
# don't do it here.
kill_current_use(compact, val)
end
if isa(val, SSAValue)
phi_uses[val.id] -= 1
end
end
end
if delete_inst
compact[ssa] = nothing
elseif !isempty(to_drop)
deleteat!(phi.values, to_drop)
deleteat!(phi.edges, to_drop)
end
end

"""
adce_pass!(ir::IRCode) -> newir::IRCode
Expand Down Expand Up @@ -1596,7 +1620,8 @@ function adce_pass!(ir::IRCode)
phi = unionphi[1]
t = unionphi[2]
if t === Union{}
compact[SSAValue(phi)] = nothing
stmt = compact[SSAValue(phi)][:inst]::PhiNode
kill_phi!(compact, phi_uses, 1:length(stmt.values), SSAValue(phi), stmt, true)
continue
elseif t === Any
continue
Expand All @@ -1617,12 +1642,7 @@ function adce_pass!(ir::IRCode)
end
end
compact.result[phi][:type] = t
isempty(to_drop) && continue
for d in to_drop
isassigned(stmt.values, d) && kill_current_use(compact, stmt.values[d])
end
deleteat!(stmt.values, to_drop)
deleteat!(stmt.edges, to_drop)
kill_phi!(compact, phi_uses, to_drop, SSAValue(phi), stmt, false)
end
# Perform simple DCE for unused values
extra_worklist = Int[]
Expand Down
10 changes: 10 additions & 0 deletions test/compiler/irpasses.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1092,3 +1092,13 @@ let src = code_typed1(foo_defined_last_iter, Tuple{Int})
end
end
end

# Issue #47180, incorrect phi counts in CmdRedirect
function a47180(b; stdout )
c = setenv(b, b.env)
if true
c = pipeline(c, stdout)
end
c
end
@test isa(a47180(``; stdout), Base.AbstractCmd)

0 comments on commit c2dfe1d

Please sign in to comment.