cfg_simplify: Correctly handle must-throw blocks #54216
Conversation
We have a special exception in our IR that allows `::Union{}` marked
instructions to act as an `unreachable` terminator.
Without this fix, we were advancing into the subsequent block despite it
not being a successor.
|
|
||
| Core.Compiler.verify_ir(ir) | ||
| ir = Core.Compiler.cfg_simplify!(ir) | ||
| @test length(ir.cfg.blocks) == 3 # should have removed block 3 |
There was a problem hiding this comment.
Verify the IR here again?
On a general note I think we have a predicate is_terminator or something similar and I don't think it handles this correctly
There was a problem hiding this comment.
Yeah, it doesn't - It has the same problem compute_basic_blocks does, which is that it expects to be able to determine a terminator from the expression alone but since #49797 it depends on the type as well
There was a problem hiding this comment.
I wonder if instead we should insert an unreachable terminator instead. Normalizing the IR more.
There was a problem hiding this comment.
I'd be in support of that, but I believe @Keno is trying to avoid making any insertions in irinterp
There was a problem hiding this comment.
Terminator normalization is #41476, but @topolarity is right, I don't want to make irinterp structurally mutating (and thus statement-index invalidating) just for this.
topolarity
force-pushed
the
ct/cfg-simplify-unreachable
branch
2 times, most recently
from
d00612b to
2d05d41
Compare
topolarity
force-pushed
the
ct/cfg-simplify-unreachable
branch
from
2d05d41 to
cab4f53
Compare
test/compiler/irpasses.jl
Outdated
| ir = make_ircode(code; ssavaluetypes=Any[Union{}, Union{}, Any, Union{}]) | ||
|
|
||
| # Unfortunately `compute_basic_blocks` does not notice the `throw()` so it gives us | ||
| # the wrong CFG. Instead manually construct the intended CFG: |
There was a problem hiding this comment.
I wouldn't say the CFG is wrong. It's valid to have extra unreachable bits at the end of the basic block.
There was a problem hiding this comment.
Right, it's not an invalid CFG - It's just not the (more precise) one we need for this test.
I'll clarify in the comment
There was a problem hiding this comment.
| # the wrong CFG. Instead manually construct the intended CFG: | |
| # a slightly imprecise CFG. Instead manually construct the CFG we need for this test: |
There was a problem hiding this comment.
Thinking on this again, unfortunately I think this CFG is wrong in general, for the same reasons we encountered in #53512. Since Enter/Exit nodes add structural conditions to the CFG, spurious fallthroughs are invalid in general.
I think we might be safe from hitting this in practice because we end up not having these implicit must-throw terminators in fresh code going into inference, but nonetheless we probably need to fix that...
oscardssmith
added
bugfix
The check added JuliaLang#54216 failed to consider the `Union{}` type that typically appears on explicit terminators. This was causing this loop to exit early for GotoIfNot nodes, which then led them to not schedule their fallthrough successors successfully.
The check added in JuliaLang#54216 failed to consider that explicit terminators also carry a `Union{}` type, not just ϕ-nodes and non-terminators. This was causing GotoIfNot nodes to exit scheduling early and fail to assign a contiguous index for their fallthrough successor.
…inator This addresses a bug in #54216, where a `GotoIfNot` was accidentally considered a throwing terminator. Just as I was about to PR this, I noticed that #54260 had already been opened for the same issue. However, there's three differences in this PR that made me open it anyway: 1. There's an additional missing case where the terminator is `nothing` which has special case semantics of allowing any type on it, because it serves as a deletion marker. 2. My test also test the `EnterNode` and `:leave` terminators, just to have a complete sampling. 3. I like the code flow in this version slightly better. Co-authored-by: Cody Tapscott <cody.tapscott@juliahub.com>
…inator This addresses a bug in #54216, where a `GotoIfNot` was accidentally considered a throwing terminator. Just as I was about to PR this, I noticed that #54260 had already been opened for the same issue. However, there's three differences in this PR that made me open it anyway: 1. There's an additional missing case where the terminator is `nothing` which has special case semantics of allowing any type on it, because it serves as a deletion marker. 2. My test also test the `EnterNode` and `:leave` terminators, just to have a complete sampling. 3. I like the code flow in this version slightly better. Co-authored-by: Cody Tapscott <cody.tapscott@juliahub.com>
…inator This addresses a bug in #54216, where a `GotoIfNot` was accidentally considered a throwing terminator. Just as I was about to PR this, I noticed that #54260 had already been opened for the same issue. However, there's three differences in this PR that made me open it anyway: 1. There's an additional missing case where the terminator is `nothing` which has special case semantics of allowing any type on it, because it serves as a deletion marker. 2. My test also test the `EnterNode` and `:leave` terminators, just to have a complete sampling. 3. I like the code flow in this version slightly better. Co-authored-by: Cody Tapscott <cody.tapscott@juliahub.com>
…inator (#54262) This addresses a bug in #54216, where a `GotoIfNot` was accidentally considered a throwing terminator. Just as I was about to PR this, I noticed that #54260 had already been opened for the same issue. However, there's three differences in this PR that made me open it anyway: 1. There's an additional missing case where the terminator is `nothing` which has special case semantics of allowing any type on it, because it serves as a deletion marker. 2. My test also test the `EnterNode` and `:leave` terminators, just to have a complete sampling. 3. I like the code flow in this version slightly better. Co-authored-by: Cody Tapscott <cody.tapscott@juliahub.com>
4 participants
We have a special exception in our IR that allows
::Union{}marked instructions to act as anunreachableterminator.Without this fix, we were advancing into the subsequent block despite it not being a successor.