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

Update to changes to construct_ssa #234

Merged
merged 2 commits into from
Nov 23, 2023
Merged

Update to changes to construct_ssa #234

merged 2 commits into from
Nov 23, 2023

Conversation

oxinabox
Copy link
Member

This is an update to make things work after JuliaLang/julia#50943

Right now however it seems to makes tests hang

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

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

Comparison is base (7be0a92) 56.17% compared to head (d63aeed) 56.50%.
Report is 7 commits behind head on main.

Files Patch % Lines
src/stage2/abstractinterpret.jl 25.00% 3 Missing ⚠️
src/stage1/recurse.jl 66.66% 1 Missing ⚠️
src/stage2/interpreter.jl 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #234      +/-   ##
==========================================
+ Coverage   56.17%   56.50%   +0.32%     
==========================================
  Files          28       28              
  Lines        2848     2869      +21     
==========================================
+ Hits         1600     1621      +21     
  Misses       1248     1248              

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

Copy link
Contributor

@topolarity topolarity left a comment

Choose a reason for hiding this comment

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

LGTM

The only thing that construct_ssa! inspects in the OptimizationState are the function- and block- level slot types (slottypes and bb_vartables, respectively)

@@ -276,7 +276,13 @@ function optic_transform!(ci, mi, nargs, N)
stmts = VERSION < v"1.11.0-DEV.258" ? ir.stmts.inst : ir.stmts.stmt
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW in the typical pipeline convert_to_ircode is used to convert CodeInfo-style statements to IRCode, but that's skipped here.

Most of the function is a no-op if you have no (informative) results from inference but there is some normalization (e.g. strip_trailing_junk!) that might be needed here for certain IR.

None of that is new in this PR though, so we can worry about it later.

@oxinabox
Copy link
Member Author

@topolarity any ideas why this would make it hang?
Seen both locally and in CI

@topolarity
Copy link
Contributor

topolarity commented Oct 18, 2023

Not sure, but here's a reduced MWE at least:

using Diffractor

function times_three_while(x)
    z = x
    i = 3
    while i > 1
        z += x
        i -= 1
    end
    z
end

let var"'" = Diffractor.PrimeDerivativeBack
    println(times_three_while'(1.0) == 3.0)
end

The IR that Diffractor generates is identical on latest Julia versus 2cee483bce (ir.txt), but inference appears to get stuck in a loop doing semi-concrete eval of > and -:

semi-concrete eval: (::Diffractor.∂⃖{1})(typeof(Base.:(-)), Int64, Int64) from (::Diffractor.∂⃖{N})(T, Any...) where {T, N}
semi-concrete eval: (::Diffractor.∂⃖{1})(typeof(Base.:(>)), Int64, Int64) from (::Diffractor.∂⃖{N})(T, Any...) where {T, N}
semi-concrete eval: (::Diffractor.∂⃖{1})(typeof(Base.:(-)), Int64, Int64) from (::Diffractor.∂⃖{N})(T, Any...) where {T, N}
semi-concrete eval: (::Diffractor.∂⃖{1})(typeof(Base.:(>)), Int64, Int64) from (::Diffractor.∂⃖{N})(T, Any...) where {T, N}
semi-concrete eval: (::Diffractor.∂⃖{1})(typeof(Base.:(-)), Int64, Int64) from (::Diffractor.∂⃖{N})(T, Any...) where {T, N}
semi-concrete eval: (::Diffractor.∂⃖{1})(typeof(Base.:(>)), Int64, Int64) from (::Diffractor.∂⃖{N})(T, Any...) where {T, N}
... (repeats infinitely)

@oxinabox
Copy link
Member Author

I guess since the IR is identical, we could consider merging this.

Particularily the update for `CC.finish!` is essential.
@oxinabox oxinabox closed this Nov 3, 2023
@oxinabox oxinabox reopened this Nov 3, 2023
@aviatesk aviatesk closed this Nov 23, 2023
@aviatesk aviatesk reopened this Nov 23, 2023
@aviatesk
Copy link
Member

Going to merge as the 1.10 CI passes.

@aviatesk aviatesk merged commit 146ae3d into main Nov 23, 2023
10 of 20 checks passed
@aviatesk aviatesk deleted the ox/nirevfix branch November 23, 2023 07:56
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