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.

Sign up for GitHub

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

Jump to bottom

effects: complements #43852, implement effect override mechanisms #44561

Merged
merged 1 commit into from
Mar 15, 2022
Merged

effects: complements #43852, implement effect override mechanisms #44561

merged 1 commit into from
Mar 15, 2022

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Mar 11, 2022 •
edited
Loading

The PR #43852 missed to implement the mechanism to override analyzed
effects with effect settings annotated by Base.@assume_effects.
This commits adds such an mechanism within finish(::InferenceState, ::AbstractInterpreter),
just after inference analyzed frame effect.

Now we can do something like:

Base.@assume_effects :consistent :effect_free :terminates_globally consteval(
    f, args...; kwargs...) = f(args...; kwargs...)
const ___CONST_DICT___ = Dict{Any,Any}(:a => 1, :b => 2)
@test fully_eliminated() do
    consteval(getindex, ___CONST_DICT___, :a)
end

@aviatesk aviatesk requested a review from Keno March 11, 2022 11:04
@aviatesk aviatesk added the backport 1.8 Change should be backported to release-1.8 label Mar 11, 2022
@KristofferC KristofferC mentioned this pull request Mar 11, 2022
Merged
47 tasks
@aviatesk aviatesk mentioned this pull request Mar 12, 2022
Merged
@aviatesk
Copy link
Member Author

aviatesk commented Mar 14, 2022 •
edited
Loading

@Keno could I ask your review on this again? I have more changes on #44515 locally, but since they are on top of this PR, I'd like to complete this PR soonish if possible.

@aviatesk aviatesk changed the title effects: complete edge effect overrides effects: add more edge effect overrides Mar 14, 2022
Keno
Keno reviewed Mar 14, 2022
View reviewed changes
base/compiler/abstractinterpretation.jl Outdated
@@ -606,6 +606,9 @@ function abstract_call_method(interp::AbstractInterpreter, method::Method, @nosp
if edge === nothing
edgecycle = edgelimited = true
end
edge_effects = override_edge_effects(edge_effects, method, :consistent)
edge_effects = override_edge_effects(edge_effects, method, sv, :effect_free)
Copy link
Member

Choose a reason for hiding this comment

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

Actually, upon second thought, I think the same argument as to :consistent, applies to :effect_free as well, so I think these should all just look at the callee's effects. However, in that case, I'm not completely sure why we need this at all, because the callee's effects should have already caused an override at the conclusion of inference.

Copy link
Member Author

@aviatesk aviatesk Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

However, in that case, I'm not completely sure why we need this at all, because the callee's effects should have already caused an override at the conclusion of inference.

That is because we don't respect annotated effect settings when we update effect property. So even if a callee frame is explicitly annotated as :effect_free, our effect tracking system may taint its :effect_free property anyway. This commit basically just overrides that entire result here.

And now I think we should move this effect override mechanism to tristate_merge!?

Copy link
Member

Choose a reason for hiding this comment

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

Well, I thought we were doing this overriding just after inference is done, but before writing the effects into the CodeInstance for caching? I guess that code got lost somewhere? In any case, that's the correct place to put this. The termination override here is just because otherwise you may have to taint something due to recursion, which the termination override prevents.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I will (re-)implement the override mechanism just before CodeInstance caching.

@aviatesk aviatesk changed the title effects: add more edge effect overrides effects: complements #43852, implement effect override mechanisms Mar 14, 2022
aviatesk added a commit that referenced this pull request Mar 14, 2022
@aviatesk
AbstractInterpreter: enable selective pure/concrete eval for extern…
7ae3491 
…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
```
aviatesk added a commit that referenced this pull request Mar 14, 2022
@aviatesk
AbstractInterpreter: enable selective pure/concrete eval for extern…
5207c46 
…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
```
aviatesk added a commit that referenced this pull request Mar 14, 2022
@aviatesk
AbstractInterpreter: enable selective pure/concrete eval for extern…
44ebba2 
…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
```
aviatesk added a commit that referenced this pull request Mar 14, 2022
@aviatesk
AbstractInterpreter: enable selective pure/concrete eval for extern…
53fed22 
…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
```
aviatesk added a commit that referenced this pull request Mar 14, 2022
@aviatesk
AbstractInterpreter: enable selective pure/concrete eval for extern…
013834a 
…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
```
@aviatesk
effects: complements #43852, implement effect override mechanisms
b1bd5aa 
The PR #43852 missed to implement the mechanism to override analyzed
effects with effect settings annotated by `Base.@assume_effects`.
This commits adds such an mechanism within `finish(::InferenceState, ::AbstractInterpreter)`,
just after inference analyzed frame effect.

Now we can do something like:
```julia
Base.@assume_effects :consistent :effect_free :terminates_globally consteval(
    f, args...; kwargs...) = f(args...; kwargs...)
const ___CONST_DICT___ = Dict{Any,Any}(:a => 1, :b => 2)
@test fully_eliminated() do
    consteval(getindex, ___CONST_DICT___, :a)
end
```
aviatesk added a commit that referenced this pull request Mar 14, 2022
@aviatesk
AbstractInterpreter: enable selective pure/concrete eval for extern…
f840163 
…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
```
@aviatesk aviatesk added merge me PR is reviewed. Merge when all tests are passing and removed merge me PR is reviewed. Merge when all tests are passing labels Mar 14, 2022
@aviatesk aviatesk merged commit b2890d5 into master Mar 15, 2022
@aviatesk aviatesk deleted the avi/override branch March 15, 2022 02:04
aviatesk added a commit that referenced this pull request Mar 15, 2022
@aviatesk
AbstractInterpreter: enable selective pure/concrete eval for extern…
dba4df7 
…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
```
aviatesk added a commit that referenced this pull request Mar 15, 2022
@aviatesk
effects: complements #43852, implement effect override mechanisms (#4…
510b53f 
…4561)

The PR #43852 missed to implement the mechanism to override analyzed
effects with effect settings annotated by `Base.@assume_effects`.
This commits adds such an mechanism within `finish(::InferenceState, ::AbstractInterpreter)`,
just after inference analyzed frame effect.

Now we can do something like:
```julia
Base.@assume_effects :consistent :effect_free :terminates_globally consteval(
    f, args...; kwargs...) = f(args...; kwargs...)
const ___CONST_DICT___ = Dict{Any,Any}(:a => 1, :b => 2)
@test fully_eliminated() do
    consteval(getindex, ___CONST_DICT___, :a)
end
```
aviatesk added a commit that referenced this pull request Mar 15, 2022
@aviatesk
AbstractInterpreter: enable selective pure/concrete eval for extern…
c0c14b6 
…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
```
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Keno Keno Keno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@aviatesk @Keno @KristofferC