FluxML / IRTools.jl

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

potential fix for try - branch - catch #128

Open maartenvd opened 1 month ago

maartenvd commented 1 month ago

I must admit I don't fully understand the ssa! pass, but there is an issue where try ... can branch. handlers/catch_branches will be incorrect for those branches. Example:

met = IRTools.meta(Tuple{typeof(Base.Partr.multiq_size), Int8})
ir = IRTools.IR(met)

Again, I don't fully understand the pass, and this 'fix' may break other things!

I imagine that it is possible for a branch to jump back to an earlier branch, and catch_branches will then be completely wrong!

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 75.19%. Comparing base (8f5f50e) to head (8ca9f1f). Report is 11 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #128 +/- ## ========================================== + Coverage 73.61% 75.19% +1.58% ========================================== Files 16 16 Lines 1512 1532 +20 ========================================== + Hits 1113 1152 +39 + Misses 399 380 -19 ```

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

Pangoraw commented 1 month ago

but there is an issue where try ... can branch. handlers/catch_branches

yeah, the ssa! pass seems to be written with the assumption that there is only one leave per try region (+1 leave in the catch region) which is not the case when there is a return in the try.

Here is a minimal reproducer which you can add as a test:

function f(cond)
   try
       if cond return end
   finally
   end
end

I am not sure about this fix, though