optimize: Delete incoming unreachable edges from PhiNode #53877
Conversation
aviatesk
force-pushed
the
kf/phinodeinvariant
branch
from
ec970c3 to
714bc4d
Compare
base/compiler/optimize.jl
Outdated
| new_edges = Int32[] | ||
| new_vals = Any[] | ||
| for j = 1:length(expr.edges) | ||
| (expr.edges[j] in sv.unreachable) && continue |
There was a problem hiding this comment.
| (expr.edges[j] in sv.unreachable) && continue | |
| (expr.edges[j] in sv.unreachable || ssavaluetypes[expr.edges[j]] === Union{}) && continue |
Ah, it turns out expr.edges[j] in sv.unreachable is not enough, since must-throw statement can be a fall-through terminator.
There was a problem hiding this comment.
Later in the function we spell this as ssavaluetypes[idx] === Union{} && !(oldidx in sv.unreachable) && !isa(code[idx], PhiNode), which suggests this probably needs the PhiNode case as well, or rather we should factor out the query and use it in both places.
There was a problem hiding this comment.
is_must_throw(idx::Int) = ssavaluetypes[idx] === Union{}?
There was a problem hiding this comment.
Well, I was suggesting factoring out all three conditions, but it's slightly awkward since the use later in the function is also renumbering things, so there's multiple index sets. I guess for now maybe just add the PhiNode condition here (and ideally a test for it).
There was a problem hiding this comment.
Hmm, what does 'The PhiNode condition' mean? At L1159, !isa(code[idx], PhiNode) is used to check if the statement being rewritten is a PhiNode. In this context, the statement being rewritten is PhiNode always, and I think the logic ((edge in sv.unreachable || is_must_throw(edge))) still applies even when code[edge] isa PhiNode.
There was a problem hiding this comment.
If the code[edge] is a phi node, its ssavaluetype can be Union{}, without that implying that the statement throws, so it needs to be excluded from that check just like on the later line
There was a problem hiding this comment.
Understood, thanks for the explanation. I have no objections to putting in that check, but I found it challenging to come up with a test case. It seems to be a pretty rare scenario, so maybe it's alright to go ahead without it.
Incoming IR very rarely contains PhiNodes, but we do allow it to make things easier on downstream packages like Diffractor that want to generate the same code structures in both typed and untyped mode. However, this does of course mean that once inference is finished, any PhiNodes in the original source must be adjusted to maintain IRCode invariants. One particular important invariant here is that the edges list in a PhiNode must match the predecessor list, so in particular if a predecessor becomes unreachable during inference, we must filter that edge before passing it on to the optimizer.
aviatesk
force-pushed
the
kf/phinodeinvariant
branch
from
fc3683d to
91f388c
Compare
Incoming IR very rarely contains PhiNodes, but we do allow it to make things easier on downstream packages like Diffractor that want to generate the same code structures in both typed and untyped mode. However, this does of course mean that once inference is finished, any PhiNodes in the original source must be adjusted to maintain IRCode invariants. One particular important invariant here is that the edges list in a PhiNode must match the predecessor list, so in particular if a predecessor becomes unreachable during inference, we must filter that edge before passing it on to the optimizer.