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

AbstractInterpreter: enable selective pure/concrete eval for external AbstractInterpreter with overlayed method table #44515

Merged
merged 2 commits into from
Mar 15, 2022

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Mar 8, 2022

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:

@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 prevent concrete-eval in the following kind of situation:

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

@aviatesk aviatesk added the backport 1.8 Change should be backported to release-1.8 label Mar 8, 2022
@aviatesk aviatesk requested a review from vtjnash March 8, 2022 11:36
@vchuravy vchuravy added this to the 1.8 milestone Mar 8, 2022
@maleadt
Copy link
Member

maleadt commented Mar 8, 2022

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.

@vtjnash vtjnash requested review from Keno and removed request for vtjnash March 8, 2022 19:52
@aviatesk aviatesk force-pushed the avi/follow44448 branch 2 times, most recently from fe2f998 to e6311dd Compare March 9, 2022 02:25
Base automatically changed from avi/follow44448 to master March 9, 2022 07:34
@aviatesk aviatesk mentioned this pull request Mar 9, 2022
47 tasks
aviatesk added a commit that referenced this pull request Mar 11, 2022
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
```
aviatesk added a commit that referenced this pull request Mar 11, 2022
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
```
aviatesk added a commit that referenced this pull request Mar 14, 2022
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
```
@aviatesk aviatesk changed the base branch from master to avi/override March 14, 2022 03:59
@aviatesk aviatesk force-pushed the avi/override branch 2 times, most recently from f894fba to f5f7445 Compare March 14, 2022 05:06
@aviatesk aviatesk changed the title AbstractInterpreter: enable partial pure/concrete eval for external AbstractInterpreter with overlayed method table AbstractInterpreter: enable selective pure/concrete eval for external AbstractInterpreter with overlayed method table Mar 14, 2022
@aviatesk
Copy link
Member Author

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 runbenchmarks("inference", vs=":master")

@@ -640,8 +671,8 @@ end

function pure_eval_eligible(interp::AbstractInterpreter,
@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
Copy link
Member Author

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

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@aviatesk aviatesk force-pushed the avi/partialeval branch 2 times, most recently from f88b3e4 to 49a3f14 Compare March 14, 2022 11:15
Base automatically changed from avi/override to master March 15, 2022 02:04
@aviatesk
Copy link
Member Author

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.

aviatesk and others added 2 commits March 15, 2022 13:29
…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.
@aviatesk aviatesk merged commit 5e4abce into master Mar 15, 2022
@aviatesk aviatesk deleted the avi/partialeval branch March 15, 2022 09:26
aviatesk added a commit that referenced this pull request Mar 15, 2022
`AbstractInterpreter`: enable selective concrete-evaluation for external `AbstractInterpreter` with overlayed method table
aviatesk added a commit to aviatesk/GPUCompiler.jl that referenced this pull request Mar 15, 2022
aviatesk added a commit to JuliaDebug/JuliaInterpreter.jl that referenced this pull request Mar 15, 2022
(e.effect_free.state << 2) |
(e.nothrow.state << 4) |
(e.terminates.state << 6)
return (e.consistent.state << 1) |
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 bigger than a UInt8 now.

Copy link
Member

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.

Copy link
Member Author

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
85

Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

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.

@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Mar 15, 2022
aviatesk added a commit to JuliaDebug/JuliaInterpreter.jl that referenced this pull request Mar 16, 2022
aviatesk added a commit to aviatesk/GPUCompiler.jl that referenced this pull request Mar 16, 2022
Especially this commits adds the update corresponding to 
<JuliaLang/julia#44515>.
maleadt pushed a commit to aviatesk/GPUCompiler.jl that referenced this pull request Mar 16, 2022
Especially this commits adds the update corresponding to 
<JuliaLang/julia#44515>.
maleadt added a commit to JuliaGPU/GPUCompiler.jl that referenced this pull request Mar 16, 2022
* 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 <tim.besard@gmail.com>
Keno pushed a commit that referenced this pull request Oct 9, 2023
Especially this commit adds the update corresponding to JuliaLang/julia/#44515.
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.

7 participants