search

FluxML / IRTools.jl

Mike's Little Intermediate Representation
MIT License
111 stars 35 forks source link

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

Closed Pangoraw closed 10 months ago

Pangoraw commented 10 months ago

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.

codecov-commenter commented 10 months ago

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%.

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

Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/FluxML/IRTools.jl/pull/115?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux) | Coverage Δ | | |---|---|---| | [src/reflection/utils.jl](https://app.codecov.io/gh/FluxML/IRTools.jl/pull/115?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux#diff-c3JjL3JlZmxlY3Rpb24vdXRpbHMuamw=) | `89.65% <100.00%> (ø)` | | | [src/ir/wrap.jl](https://app.codecov.io/gh/FluxML/IRTools.jl/pull/115?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Flux#diff-c3JjL2lyL3dyYXAuamw=) | `93.75% <50.00%> (+0.05%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

simeonschaub commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

https://github.com/JuliaLang/julia/pull/51715 is the fix for find_ssavalue_uses.

simeonschaub commented 10 months ago

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 commented 10 months ago

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... :laughing: