-
-
Couldn't load subscription status.
- Fork 5.7k
AbstractInterpreter: enable selective pure/concrete eval for external AbstractInterpreter with overlayed method table
#44515
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
Conversation
|
I'm not familiar enough with this code to review, but it does fix the GPUCompiler issue and makes CUDA.jl tests pass on 1.9. |
fe2f998 to
e6311dd
Compare
81c0cd1 to
6635c1b
Compare
6635c1b to
7f1e461
Compare
While working on #44515, I found we missed to override edge effect with effects set by `Base.@assume_effects`. This commit adds edge effect override mechanism for `:consistent`/`:effect_free`/`:nothrow` settings in the same way as we did for `:terminates_globally` in #44106. Now we can do something like: ```julia const ___CONST_DICT___ = Dict{Any,Any}(:a => 1, :b => 2) Base.@assume_effects :consistent :effect_free :terminates_globally consteval(f, args...) = f(args...) @test fully_eliminated() do consteval(getindex, ___CONST_DICT___, :a) end ```
8ee6570 to
af1e6ca
Compare
While working on #44515, I found we missed to override edge effect with effects set by `Base.@assume_effects`. This commit adds edge effect override mechanism for `:consistent`/`:effect_free`/`:nothrow` settings in the same way as we did for `:terminates_globally` in #44106. Now we can do something like: ```julia const ___CONST_DICT___ = Dict{Any,Any}(:a => 1, :b => 2) Base.@assume_effects :consistent :effect_free :terminates_globally consteval(f, args...) = f(args...) @test fully_eliminated() do consteval(getindex, ___CONST_DICT___, :a) end ```
While working on #44515, I found we missed to override edge effect with effects set by `Base.@assume_effects`. This commit adds edge effect override mechanism for `:consistent`/`:effect_free`/`:nothrow` settings in the same way as we did for `:terminates_globally` in #44106. Now we can do something like: ```julia const ___CONST_DICT___ = Dict{Any,Any}(:a => 1, :b => 2) Base.@assume_effects :consistent :effect_free :terminates_globally consteval(f, args...) = f(args...) @test fully_eliminated() do consteval(getindex, ___CONST_DICT___, :a) end ```
af1e6ca to
90314dd
Compare
f894fba to
f5f7445
Compare
90314dd to
6f90d51
Compare
AbstractInterpreter: enable partial pure/concrete eval for external AbstractInterpreter with overlayed method tableAbstractInterpreter: enable selective pure/concrete eval for external AbstractInterpreter with overlayed method table
6f90d51 to
f492e34
Compare
|
Ok, the entire PRs (this one and #44561) ended up being bigger than I expected, but I think this is ready for another review. @nanosoldier |
| @nospecialize(f), applicable::Vector{Any}, arginfo::ArgInfo, sv::InferenceState) | ||
| return !isoverlayed(method_table(interp)) && | ||
| f !== nothing && | ||
| # XXX we need to check that this pure function doesn't call any overlayed method |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For correctness, we need to look at the effect of the pure function being analyzed here and assert that it doesn't involve any overlayed method call within it.
But I think a pure function isn't really supposed to call a generic function (which may be overlayed) anyway, and so here I'd like to avoid trying hard to exclude such rare and invalid usecases and adding implementation complexity
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
f492e34 to
9375f3b
Compare
f88b3e4 to
49a3f14
Compare
49a3f14 to
17cbdae
Compare
17cbdae to
7d9a948
Compare
|
Rebased to take in #44561 . I'm planning to merge this once I confirm CI runs correctly as Kristoffer and I want those PR to be included in the next beta release. |
…al `AbstractInterpreter` with overlayed method table Built on top of #44511 and #44561, and solves <JuliaGPU/GPUCompiler.jl#309>. This commit allows external `AbstractInterpreter` to selectively use pure/concrete evals even if it uses an overlayed method table. More specifically, such `AbstractInterpreter` can use pure/concrete evals as far as any callees used in a call in question doesn't come from the overlayed method table: ```julia @test Base.return_types((), MTOverlayInterp()) do isbitstype(Int) ? nothing : missing end == Any[Nothing] Base.@assume_effects :terminates_globally function issue41694(x) res = 1 1 < x < 20 || throw("bad") while x > 1 res *= x x -= 1 end return res end @test Base.return_types((), MTOverlayInterp()) do issue41694(3) == 6 ? nothing : missing end == Any[Nothing] ``` In order to check if a call is tainted by any overlayed call, our effect system now additionally tracks `overlayed::Bool` property. This effect property is required to prevents concrete-eval in the following kind of situation: ```julia strangesin(x) = sin(x) @overlay OverlayedMT strangesin(x::Float64) = iszero(x) ? nothing : cos(x) Base.@assume_effects :total totalcall(f, args...) = f(args...) @test Base.return_types(; interp=MTOverlayInterp()) do # we need to disable partial pure/concrete evaluation when tainted by any overlayed call if totalcall(strangesin, 1.0) == cos(1.0) return nothing else return missing end end |> only === Nothing ```
Especially, it should be more consistent with the other reflection utilities defined in reflection.jl if the optional `interp::AbstractInterpreter` argument is passed as keyword one.
7d9a948 to
da7676d
Compare
`AbstractInterpreter`: enable selective concrete-evaluation for external `AbstractInterpreter` with overlayed method table
| (e.effect_free.state << 2) | | ||
| (e.nothrow.state << 4) | | ||
| (e.terminates.state << 6) | ||
| return (e.consistent.state << 1) | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is bigger than a UInt8 now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In #43899 I changed this to a UInt32, so you may want to pick up those pieces.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since e.overlayed is a Boolean property, I think this can be encoded within UInt8?
julia> let maxbits = 0x02
(maxbits << 1) |
(maxbits << 3) |
(maxbits << 5) |
(maxbits << 7) |
true
end |> Int
85There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, 4*2+1 is 9 bits. You're cutting of the top bit (in particular the top bit of the two bit unit you're shifting by 7).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Au, you're right. Maybe we can pick UInt16 instead though?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think UInt32 is fine. I have at least two more effects I think would be interesting, so with you're extra bit, and the one I already have in that PR, we'd be out again. It's not a huge amount of memory.
Especially this commits adds the update corresponding to <JuliaLang/julia#44515>.
Especially this commits adds the update corresponding to <JuliaLang/julia#44515>.
* adapt to upstream changes to `Base.return_types` Especially this commits adds the update corresponding to <JuliaLang/julia#44515>. * Fix test. * more update with backports Co-authored-by: Tim Besard <[email protected]>
Especially this commit adds the update corresponding to JuliaLang/julia/#44515.
Built on top of #44511 and #44561, and solves JuliaGPU/GPUCompiler.jl#309.
This commit allows external
AbstractInterpreterto selectively usepure/concrete evals even if it uses an overlayed method table.
More specifically, such
AbstractInterpretercan use pure/concrete evalsas far as any callees used in a call in question doesn't come from the
overlayed method table:
In order to check if a call is tainted by any overlayed call, our effect
system now additionally tracks
overlayed::Boolproperty. This effectproperty is required to prevent concrete-eval in the following kind of situation: