Skip to content

Commit

Permalink
Fix SROA confusing new and old nodes
Browse files Browse the repository at this point in the history
SROA was accidentally treating a pending node as old and thus
getting the wrong type when querying the predecessor. As a result
it thought one of the paths was unreachable causing undefined data
to be introduced on that path (generally the `1.0` that happened to
already be in register).

Fix #29983
  • Loading branch information
Keno authored and KristofferC committed Feb 11, 2019
1 parent 8f251a1 commit ddcee0d
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 3 deletions.
12 changes: 9 additions & 3 deletions base/compiler/ssair/passes.jl
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ function walk_to_defs(compact::IncrementalCompact, @nospecialize(defssa), @nospe
collect(Iterators.filter(1:length(def.edges)) do n
isassigned(def.values, n) || return false
val = def.values[n]
if isa(defssa, OldSSAValue) && isa(val, SSAValue)
if is_old(compact, defssa) && isa(val, SSAValue)
val = OldSSAValue(val.id)
end
edge_typ = widenconst(compact_exprtype(compact, val))
Expand All @@ -201,7 +201,7 @@ function walk_to_defs(compact::IncrementalCompact, @nospecialize(defssa), @nospe
for n in possible_predecessors
pred = def.edges[n]
val = def.values[n]
if isa(defssa, OldSSAValue) && isa(val, SSAValue)
if is_old(compact, defssa) && isa(val, SSAValue)
val = OldSSAValue(val.id)
end
if isa(val, AnySSAValue)
Expand Down Expand Up @@ -425,6 +425,12 @@ struct LiftedPhi
need_argupdate::Bool
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)
end

function perform_lifting!(compact::IncrementalCompact,
visited_phinodes::Vector{Any}, @nospecialize(cache_key),
lifting_cache::IdDict{Pair{AnySSAValue, Any}, AnySSAValue},
Expand Down Expand Up @@ -455,7 +461,7 @@ function perform_lifting!(compact::IncrementalCompact,
isassigned(old_node.values, i) || continue
val = old_node.values[i]
orig_val = val
if isa(old_node_ssa, OldSSAValue) && !is_pending(compact, old_node_ssa) && !already_inserted(compact, old_node_ssa) && isa(val, SSAValue)
if is_old(compact, old_node_ssa) && isa(val, SSAValue)
val = OldSSAValue(val.id)
end
if isa(val, Union{NewSSAValue, SSAValue, OldSSAValue})
Expand Down
26 changes: 26 additions & 0 deletions test/compiler/irpasses.jl
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,29 @@ let results = Float64[]
foo30594(4, -1)
@test results == [0.0, -1.0, -2.0, -3.0]
end

# Issue #29983
# This one is a bit hard to trigger, but the key is to create a case
# where SROA needs to introduce an intermediate type-unstable phi node
struct Foo29983{T}
x::Tuple{T}
end
struct Bar29983{S}
x::S
end
Base.:+(a::T, b::Bar29983{S}) where {T, S} = Bar29983(a + b.x)
Base.:+(a::Bar29983{S}, b::T) where {T, S} = b + a
Base.:+(a::Bar29983{S}, b::Bar29983{T}) where {T, S} = Bar29983(a.x + b.x)
Base.:+(a::Foo29983, b::Foo29983) = Foo29983((a.x[1] + b.x[1],))

function f(x::Vector{T}) where {T}
x1 = Foo29983((x[1],))
la1 = Foo29983((x[1],))
f1 = Foo29983((0,))
for _ in 1:2
f1 += la1
end
return f1
end

@test f([Bar29983(1.0)]).x[1].x == 2.0

0 comments on commit ddcee0d

Please sign in to comment.