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

fix ssa conversion for catch blocks #117

Merged
merged 8 commits into from
Dec 13, 2023

Conversation

Pangoraw
Copy link
Collaborator

@Pangoraw Pangoraw commented Dec 4, 2023

the slotused function was not enough to account for all slot used by the catch block and its successors. With this change, ssa conversion keeps a live list of all catch 'branch' instructions and fetches the reaching definitions for slots at the location of these :catch instructions.

See as an example the added test where the ir is:

function f_try_catch(x)
    y = 0.
    try
        y = sqrt(x)
    catch

    end
    y
end
# Before (catch branches have no args since slotused(block(ir,2)) is empty)
#
# 1: (%1, %2)
#   %3 = try #2
#   %4 = catch 2
#   %5 = Main.sqrt(%2)
#   %6 = catch 2
#   %7 = end try
#   br 3 (%5)
# 2: (%8)
#   %9 = end try
#   %10 = pop exception %3
#   br 3 (%8)
# 3: (%11)
#   return %11
#
# After (catch branches have args corresponding to the reaching definition of y)
#
# 1: (%1, %2)
#   %3 = try #2
#   %4 = catch 2 (0.0)
#   %5 = Main.sqrt(%2)
#   %6 = catch 2 (%5)
#   %7 = end try
#   br 3 (%5)
# 2: (%8)
#   %9 = end try
#   %10 = pop exception %3
#   br 3 (%8)
# 3: (%11)
#   return %11

the slotused function was not enough to account for all slot used by the
catch block and its successors. With this change, ssa conversion keeps
a live list of all catch 'branch' instructions and fetches the reaching
definitions for slots at the location of these :catch instructions.
@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (55c315a) 73.29% compared to head (f728dc8) 73.61%.

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

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
+ Coverage   73.29%   73.61%   +0.31%     
==========================================
  Files          16       16              
  Lines        1494     1512      +18     
==========================================
+ Hits         1095     1113      +18     
  Misses        399      399              

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


ir = @code_ir f_try_catch4(42, false)
fir = func(ir)
# This should be @test_throws UndefVarError fir(nothing,42,false)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we add a test_throws UndefVarError ... broken=true for this? (and the one above)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. It looks like @test_throws does not support broken=true yet so I had added @test try ... end broken=true instead.

Comment on lines +215 to 226
function reaching(b, slot)
haskey(defs[b.id], slot) && return defs[b.id][slot]
b.id == 1 && return undef
x = defs[b.id][v] = argument!(b, type = v.type, insert = false)
x = defs[b.id][slot] = argument!(b, type = slot.type, insert = false)
for pred in predecessors(b)
if pred.id < current
for br in branches(pred, b)
push!(br.args, reaching(pred, v))
push!(br.args, reaching(pred, slot))
end
else
push!(get!(todo[pred.id], b.id, Slot[]), v)
push!(get!(todo[pred.id], b.id, Slot[]), slot)
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is purely just a clarification of the existing code, right? to rename v to slot
(A good clarification)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! In other parts of the package, v often refers to a Variable (SSA value) so I renamed to clarify.

Copy link
Member

@oxinabox oxinabox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense.
Add the test_broken and this should be good to merge

@Pangoraw Pangoraw merged commit 8f5f50e into FluxML:master Dec 13, 2023
8 checks passed
@Pangoraw Pangoraw deleted the catch_ssa_conversion branch December 13, 2023 17:46
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