Skip to content

Commit

Permalink
Fix misoptimization with phi nodes
Browse files Browse the repository at this point in the history
  • Loading branch information
Zentrik authored and giordano committed Oct 7, 2024
1 parent abaf6e1 commit 10099d8
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 4 deletions.
27 changes: 23 additions & 4 deletions base/compiler/ssair/passes.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2172,6 +2172,7 @@ function perform_symbolic_evaluation!(key, stmt::PhiNode, ssa_to_ssa, blockidx,

firstval = nothing
allthesame = true # If all values into the phi node are the same SSAValue
has_backedge = false

ordered_indices = collect(1:no_of_edges)
sort!(ordered_indices; by=i->stmt.edges[i])
Expand All @@ -2185,6 +2186,7 @@ function perform_symbolic_evaluation!(key, stmt::PhiNode, ssa_to_ssa, blockidx,
continue
end

has_backedge |= blockidx <= edge
push!(key, edge)

if val isa SSAValue
Expand All @@ -2199,11 +2201,22 @@ function perform_symbolic_evaluation!(key, stmt::PhiNode, ssa_to_ssa, blockidx,
allthesame = false
end
end
if allthesame && firstval !== nothing &&
dominates(get!(lazydomtree), block_for_inst(ir, firstval.id), blockidx)
return firstval
if allthesame
if firstval === nothing
return svec()
end
# If we allow firstval to not be a SSAValue, we can return it now if it isn't a SSAValue

# Copy of https://github.com/llvm/llvm-project/blob/3a2f7d8a9f84db380af5122418098cb28a57443f/llvm/lib/Transforms/Scalar/NewGVN.cpp#L1796-L1804
has_undef = length(ir.cfg.blocks[blockidx].preds) != no_of_edges # undef in llvm sense

# TODO: Calculate cycle_freeness.
cycle_free = !has_backedge # Assume a backedge means there is a cycle. This is a conservative assumption.

if !(has_undef && has_backedge && !cycle_free) && dominates(get!(lazydomtree), block_for_inst(ir, firstval.id), blockidx)
return firstval
end
end
length(key) == 1 && return svec()
# returns (sorted edges, ssa_to_ssa[ssa values of sorted edges], block index)
# faster to splat a single vector into a svec than multiple,
# which is why the code above is so complex
Expand Down Expand Up @@ -2303,6 +2316,12 @@ function gvn!(ir::IRCode)
return ir
end

for (ssa_leader, class) in congruence_classes, element in class
@assert ssa_leader.id < 10000000000
@assert element.ssa < 10000000000
@assert element.blockidx < 10000000000
end

domtree = get!(lazydomtree)
dfscached_domtree = construct_dfscached_domtree(domtree)

Expand Down
22 changes: 22 additions & 0 deletions test/compiler/irpasses.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1535,6 +1535,28 @@ end
@test Core.Compiler.perform_symbolic_evaluation!(Any[], stmt, ssa_to_ssa, 3, lazydomtree, ir) == SSAValue(1)
end

# Test gvn treats phinodes depending on other phinodes in the same block correctly
let m = Meta.@lower 1 + 1
@assert Meta.isexpr(m, :thunk)
src = m.args[1]::CodeInfo
src.code = Any[
# block 1
nothing,
# block 2
PhiNode(Int32[1, 4], Any[false, true]),
PhiNode(Int32[4], Any[SSAValue(2)]),
GotoIfNot(SSAValue(2), 2),
# block 3
ReturnNode(SSAValue(3)),
]
nstmts = length(src.code)
src.ssavaluetypes = nstmts
src.ssaflags = fill(UInt8(0x00), nstmts)
src.debuginfo = Core.DebugInfo(:none)
m.args[1] = copy(src)
Core.Compiler.verify_ir(Core.Compiler.inflate_ir(src))
@test false === @eval $m
end

# Issue #51144 - UndefRefError during compaction
let code = Any[
Expand Down

0 comments on commit 10099d8

Please sign in to comment.