-
-
Notifications
You must be signed in to change notification settings - Fork 211
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
Support try/catch on the happy (nothrow) path #1474
Conversation
another possibility would be to thread the pullback values as block parameters through the control flow with arguments coming from the catch block being `nothing`.
I'm not really sure how to evaluate any risks from this change other than reading through the old PR. @oxinabox as the reviewer on that, would you be able to take a look here? |
elseif isexpr(ex, :enter, :leave) | ||
error("""try/catch is not supported. | ||
Refer to the Zygote documentation for fixes. | ||
https://fluxml.ai/Zygote.jl/latest/limitations | ||
""") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believer we still need this on versions of julia that do not have enter
and leave
?
so maybe we keep this and add a && VERSION<v"1.10"
(is that the right bounds?)
src/compiler/reverse.jl
Outdated
# This is corresponds to a catch blocks which technically | ||
# has predecessors but they are not modelled in the IRTools CFG. | ||
# We put an error message at the beginning of said block. | ||
if has_leave && isempty(predecessors(b)) && b.id != 1 | ||
_, f_stmt = first(b) | ||
li = pr.ir.lines[f_stmt.line] | ||
li = LineNumberNode(Int(li.line), li.file) | ||
pushfirst!(rb, stmt(xcall(Base, :error, | ||
"Can't differentiate function execution in catch block at $(li)."))) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think throwing the error in the catch block is right.
I think we need to put it at the throw
location.
Because I assume we also do not support finally
blocks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try finally is implemented in term of enter
and leave
which can roughly be mapped to the following pseudo code:
try
foo()
origin = 1
catch
origin = 2
end
if origin == 2
rethrow()
end
I will improve the error message to include try/finally as another construct which is not supported when an error is thrown in the block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work.
See my comments
Co-authored-by: Frames White <[email protected]>
This requires updating the IRTools version to include the better ssa conversion for try catch branches.
This comment was marked as resolved.
This comment was marked as resolved.
test/compiler.jl
Outdated
if VERSION >= v"1.8" | ||
# try/catch/else is invalid syntax prior to v1.8 | ||
eval(Meta.parse(""" | ||
function try_catch_else(cond, x) | ||
end | ||
""")) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code seems unused and incomplete
@oxinabox is this still good to merge? |
I see no reason why it wouldn't be. |
This enables differentation support to try/catch when no exception is thrown on the try block.
The changes are twofolds:
:enter
and:leave
expressions, add an dedicated error message in the adjoint of the catch block instead.Here is an example of the behavior:
FluxML/IRTools.jl#117 is a companion change that fixes some of the error that can currently happen with try/catch blocks in IRTools.jl.
Supersedes #466.