Skip to content

Commit

Permalink
sroa: Lift restriction that all_same optimization must give SSAValue
Browse files Browse the repository at this point in the history
This restriction has been in there since this code was added in #44557.
Unfortunately, I can't tell from the commit history why this restriction
was added. It's possible that it was trying to avoid putting things into
statement position that were not allowed in the phi block, but we have
cleaned that up since (#50308 and related), so let's see if this restriction
is still required, since I was seeing some suboptimial optimization results
because of this.
  • Loading branch information
Keno authored and aviatesk committed Dec 1, 2023
1 parent 58c1d51 commit e69f87d
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 18 deletions.
2 changes: 1 addition & 1 deletion base/compiler/ssair/inlining.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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, Union{SSAValue, OldSSAValue, NewSSAValue}) && return val # avoid infinite loop
urs = userefs(val)
for op in urs
op[] = ssa_substitute_op!(insert_node!, subst_inst, op[], ssa_substitute)
Expand Down
6 changes: 3 additions & 3 deletions base/compiler/ssair/ir.jl
Original file line number Diff line number Diff line change
Expand Up @@ -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{SSAValue, OldSSAValue, NewSSAValue, GlobalRef})
op == 1 || return OOB_TOKEN
return stmt
elseif isa(stmt, UpsilonNode)
Expand Down Expand Up @@ -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{SSAValue, OldSSAValue, NewSSAValue, GlobalRef})
op == 1 || throw(BoundsError())
stmt = v
elseif isa(stmt, UpsilonNode)
Expand Down Expand Up @@ -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
Expand Down
32 changes: 18 additions & 14 deletions base/compiler/ssair/passes.jl
Original file line number Diff line number Diff line change
Expand Up @@ -678,24 +678,28 @@ function perform_lifting!(compact::IncrementalCompact,
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
if all_same && isa(the_leaf, LiftedValue)
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
the_leaf_val = the_leaf.val
if isa(the_leaf_val, Union{SSAValue, OldSSAValue})
if lazydomtree === nothing
# Must conservatively assume this
dominates_all = false
else
domtree = get!(lazydomtree)
for item in visited_philikes
if !dominates_ssa(compact, domtree, the_leaf_val, item)
dominates_all = false
break
end
end
end
if dominates_all
return the_leaf
end
if dominates_all
if isa(the_leaf, OldSSAValue)
the_leaf = simple_walk(compact, the_leaf)
end
return the_leaf
end
end

Expand Down

0 comments on commit e69f87d

Please sign in to comment.