compiler: Get rid of implicit fallthrough terminators #41476
Labels
design
Design of APIs or of the language itself
Comments
Keno
added a commit
that referenced
this issue
Aug 25, 2021
Recall the reproducer from the issue:
```
julia> f() = (if nothing; end; unsafe_load(Ptr{Int}(0)))
f (generic function with 1 method)
julia> f()
Unreachable reached at 0x7fb33bb50090
signal (4): Illegal instruction
in expression starting at REPL[13]:1
unsafe_load at ./pointer.jl:105 [inlined]
unsafe_load at ./pointer.jl:105 [inlined]
```
There were actually two places where we were dropping the
GotoIfNot, one in type annotation after inference, one in
SSA conversion. The one in SSA conversion was structural:
When both branches target the same jump destination, the
GotoIfNot would be dropped. This was fine in general, except
that as shown above, GotoIfNot can actually itself have
a side effect, namely throwing a type error if the condition
is not a boolean. Thus in order to actually drop the node
we need to prove that the error check does not fire.
The reason we want to drop the GotoIfNot node here is
that IRCode has an invariant that every basic block is
in the predecessor list only once (otherwise PhiNodes
would have to carry extra state regarding which branch
they refer to).
To fix this, on option would be to create a new expr
(e.g. `boolcheck`) that works like `:throw_undef_if_not`.
However, on second thought, I figured we might as well
just re-use GotoIfNot for this, since we'd otherwise
have to duplicate all the checks for side effects and
add codegen, etc., so this does the following:
The `dest` label in GotoIfNot becomes optional. If set
to 0, the only thing the GotoIfNot does is to perform
the boolcheck. It is also no longer considered a terminator
for SSA construction, either. The regular code for
determining side-effects of GotoIfNot will determine
the bool-ness of the condition and delete the GotoIfNot
if applicable.
This also aligns with another PR I'm working on, which
will add an optional `else` label to GotoIfNot (in
the direction of #41476). Hopefully with this, the
two paths will then be symmetric.
Keno
added a commit
that referenced
this issue
May 12, 2023
This is yet another followup to #49692 and #49750. With the introduced change, we kill the CFG edge from the basic block with the discovered error to its successors. However, we have an invariant in the verifier that the CFG should always match the IR. Turns out this is for good reason, as we assume in a number of places (including, ironically in the irinterp) that a GotoNode/GotoIfNot terminator means that the BB has the corresponding number of successors in the IR. Fix all this by killing the rest of the basic block when we discover that it is unreachable and if possible introducing an unreachable node at the end. However, of course if the erroring statement is the fallthrough terminator itself, there is no space for an unreachable node. We fix this by tweaking the verification to allow this case, as its really no worse than the other problems with fall-through terminators (#41476), but of course it would be good to address that as part of a more general IR refactor.
Keno
added a commit
that referenced
this issue
May 13, 2023
This is yet another followup to #49692 and #49750. With the introduced change, we kill the CFG edge from the basic block with the discovered error to its successors. However, we have an invariant in the verifier that the CFG should always match the IR. Turns out this is for good reason, as we assume in a number of places (including, ironically in the irinterp) that a GotoNode/GotoIfNot terminator means that the BB has the corresponding number of successors in the IR. Fix all this by killing the rest of the basic block when we discover that it is unreachable and if possible introducing an unreachable node at the end. However, of course if the erroring statement is the fallthrough terminator itself, there is no space for an unreachable node. We fix this by tweaking the verification to allow this case, as its really no worse than the other problems with fall-through terminators (#41476), but of course it would be good to address that as part of a more general IR refactor.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This has been discussed on slack a bit, but the implicit fallthrough terminator design is
pretty terrible from the point of view of a user actually trying to do CFG manipulations.
The main problems I see with it are:
GotoIfNotis not particularly symmetric and operations that operate on it need two separate code paths, depending on whether the case they're interested in is the fallthrough or the explicit terminator.In general our IR data structures work ok for what we need in base, but are quite problematic for general compiler needs. We should see what part of this needs a refactor in Base and what needs to be provided by external packages.
The text was updated successfully, but these errors were encountered: