-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #41975 - Dropped typecheck in GotoIfNot #42010
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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, insert an `Expr(:call, typecheck, _, Bool)` when dropping the GotoIfNot. We do lose the ability to distinguish the GotoIfNot from literal typechecks as a result, but at the moment they generate identical errors. If we ever wanted to dinstinguish them, we could create another typecheck intrinsic that throws a different error or use an approach like #41994.
vtjnash
approved these changes
Aug 25, 2021
vtjnash
added
backport 1.6
Change should be backported to release-1.6
backport 1.7
merge me
PR is reviewed. Merge when all tests are passing
labels
Aug 25, 2021
aviatesk
approved these changes
Aug 26, 2021
KristofferC
pushed a commit
that referenced
this pull request
Aug 27, 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, insert an `Expr(:call, typecheck, _, Bool)` when dropping the GotoIfNot. We do lose the ability to distinguish the GotoIfNot from literal typechecks as a result, but at the moment they generate identical errors. If we ever wanted to dinstinguish them, we could create another typecheck intrinsic that throws a different error or use an approach like #41994. (cherry picked from commit 2445000)
75 tasks
KristofferC
pushed a commit
that referenced
this pull request
Aug 27, 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, insert an `Expr(:call, typecheck, _, Bool)` when dropping the GotoIfNot. We do lose the ability to distinguish the GotoIfNot from literal typechecks as a result, but at the moment they generate identical errors. If we ever wanted to dinstinguish them, we could create another typecheck intrinsic that throws a different error or use an approach like #41994. (cherry picked from commit 2445000)
63 tasks
KristofferC
pushed a commit
that referenced
this pull request
Aug 31, 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, insert an `Expr(:call, typecheck, _, Bool)` when dropping the GotoIfNot. We do lose the ability to distinguish the GotoIfNot from literal typechecks as a result, but at the moment they generate identical errors. If we ever wanted to dinstinguish them, we could create another typecheck intrinsic that throws a different error or use an approach like #41994. (cherry picked from commit 2445000)
KristofferC
pushed a commit
that referenced
this pull request
Sep 3, 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, insert an `Expr(:call, typecheck, _, Bool)` when dropping the GotoIfNot. We do lose the ability to distinguish the GotoIfNot from literal typechecks as a result, but at the moment they generate identical errors. If we ever wanted to dinstinguish them, we could create another typecheck intrinsic that throws a different error or use an approach like #41994. (cherry picked from commit 2445000)
DilumAluthge
removed
the
merge me
PR is reviewed. Merge when all tests are passing
label
Sep 6, 2021
KristofferC
removed
backport 1.6
Change should be backported to release-1.6
backport 1.7
labels
Sep 7, 2021
LilithHafner
pushed a commit
to LilithHafner/julia
that referenced
this pull request
Feb 22, 2022
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, insert an `Expr(:call, typecheck, _, Bool)` when dropping the GotoIfNot. We do lose the ability to distinguish the GotoIfNot from literal typechecks as a result, but at the moment they generate identical errors. If we ever wanted to dinstinguish them, we could create another typecheck intrinsic that throws a different error or use an approach like JuliaLang#41994.
LilithHafner
pushed a commit
to LilithHafner/julia
that referenced
this pull request
Mar 8, 2022
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, insert an `Expr(:call, typecheck, _, Bool)` when dropping the GotoIfNot. We do lose the ability to distinguish the GotoIfNot from literal typechecks as a result, but at the moment they generate identical errors. If we ever wanted to dinstinguish them, we could create another typecheck intrinsic that throws a different error or use an approach like JuliaLang#41994.
staticfloat
pushed a commit
that referenced
this pull request
Dec 23, 2022
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, insert an `Expr(:call, typecheck, _, Bool)` when dropping the GotoIfNot. We do lose the ability to distinguish the GotoIfNot from literal typechecks as a result, but at the moment they generate identical errors. If we ever wanted to dinstinguish them, we could create another typecheck intrinsic that throws a different error or use an approach like #41994. (cherry picked from commit 2445000)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Recall the reproducer from the issue:
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, insert an
Expr(:call, typecheck, _, Bool)
when dropping the GotoIfNot. We do lose the ability to
distinguish the GotoIfNot from literal typechecks as
a result, but at the moment they generate identical
errors. If we ever wanted to dinstinguish them, we could
create another typecheck intrinsic that throws a different
error or use an approach like #41994.