Skip to content
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

Use Core.throw("unreachable") for unreachables instead of ReturnNode() #115

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

Pangoraw
Copy link
Collaborator

@Pangoraw Pangoraw commented Oct 15, 2023

It is currently not possible to use IRTools.unreachable branches when building irs (see the test which fails with current IRTools).

Indeed, ReturnNode() only appears later in the Julia compiler optimization pipeline and is not expected to be part of a lowered piece of code. The Julia compiler throws in find_ssavalues_uses and this comment makes me think that it is more supported to throw than to use ReturnNode() in untyped ir.

`ReturnNode()` only appears later in the Julia compiler optimization pipeline.
@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6fcac25) 73.41% compared to head (0116a94) 73.43%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
+ Coverage   73.41%   73.43%   +0.01%     
==========================================
  Files          16       16              
  Lines        1486     1487       +1     
==========================================
+ Hits         1091     1092       +1     
  Misses        395      395              
Files Coverage Δ
src/reflection/utils.jl 89.65% <100.00%> (ø)
src/ir/wrap.jl 93.75% <50.00%> (+0.05%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@simeonschaub
Copy link
Member

I'm ok with this as a workaround, but if the issue is really with find_ssavalue_uses I'd prefer just adding an isdefined there instead. My worry is that introducing an error path here might negatively impact inference results

@Pangoraw
Copy link
Collaborator Author

Pangoraw commented Oct 15, 2023

I will try to see if there are other blocking points after adding the isdefined check in find_ssavalues_uses. One advantage with throwing is that it should work for previous julia versions.

If the code is truly unreachable, it should be removed in any case by the optimizer.

@Pangoraw
Copy link
Collaborator Author

JuliaLang/julia#51715 is the fix for find_ssavalue_uses.

Keno pushed a commit to JuliaLang/julia that referenced this pull request Oct 16, 2023
IRTools.jl currently tries to use `ReturnNode()` to model unreachable
block terminators but it fails in `find_ssavalue_uses`. This PR adds a
check to enable using `ReturnNode()` in untyped code.

One other alternative for frontends is to use an instruction known to
terminate (`Core.throw`) instead. See
FluxML/IRTools.jl#115 for more context.
src/ir/wrap.jl Outdated Show resolved Hide resolved
Co-authored-by: Simeon Schaub <[email protected]>
@simeonschaub
Copy link
Member

Tests with #116 are passing, so I will go ahead and merge this. Thanks for the great contribution! The method override warnings on nightly still sound like a potential red herring, so would be good to investigate those at some point as well.

@simeonschaub simeonschaub merged commit 2cb1227 into FluxML:master Oct 16, 2023
@simeonschaub
Copy link
Member

TFW you thought you just fixed all issues on nightly and then the Windows runner just happens to catch the build one Julia commit further up the line and everything is broken again... 😆

@Pangoraw Pangoraw deleted the throw_unreachable branch October 17, 2023 06:47
KristofferC pushed a commit to JuliaLang/julia that referenced this pull request Oct 23, 2023
IRTools.jl currently tries to use `ReturnNode()` to model unreachable
block terminators but it fails in `find_ssavalue_uses`. This PR adds a
check to enable using `ReturnNode()` in untyped code.

One other alternative for frontends is to use an instruction known to
terminate (`Core.throw`) instead. See
FluxML/IRTools.jl#115 for more context.

(cherry picked from commit ff03e51)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants