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

inference: don't forget to add backedge for effect-free frame #45017

Merged
merged 1 commit into from
Apr 19, 2022

Conversation

aviatesk
Copy link
Member

Now our abstract interpretation system caches two separate lattice
properties return type and call effects, and the previous optimization
to avoid adding backedge for a frame whose return type is Any turns
out to be insufficient -- now we also need to make sure that the
inferred effects don't provide any useful IPO information.

Example:

julia> const CONST_DICT = let d = Dict()
           for c in 'A':'z'
               push!(d, c => Int(c))
           end
           d
       end;

julia> Base.@assume_effects :total_may_throw getcharid(c) = CONST_DICT[c];

julia> @noinline callf(f, args...) = f(args...);

julia> function entry_to_be_invalidated(c)
           return callf(getcharid, c)
       end;

julia> @test Base.infer_effects((Char,)) do x
           entry_to_be_invalidated(x)
       end |> Core.Compiler.is_concrete_eval_eligible
Test Passed

julia> @test fully_eliminated(; retval=97) do
           entry_to_be_invalidated('a')
       end
Test Passed

julia> getcharid(c) = CONST_DICT[c]; # now this is not eligible for concrete evaluation

julia> @test Base.infer_effects((Char,)) do x
           entry_to_be_invalidated(x)
       end |> !Core.Compiler.is_concrete_eval_eligible
Test Failed at REPL[10]:1
  Expression: Base.infer_effects((Char,)) do x
        entry_to_be_invalidated(x)
    end |> !(Core.Compiler.is_concrete_eval_eligible)
ERROR: There was an error during testing

julia> @test !fully_eliminated() do
           entry_to_be_invalidated('a')
       end
Test Failed at REPL[11]:1
  Expression: !(fully_eliminated() do
        entry_to_be_invalidated('a')
    end)
ERROR: There was an error during testing

Now our abstract interpretation system caches two separate lattice
properties return type and call effects, and the previous optimization
to avoid adding backedge for a frame whose return type is `Any` turns
out to be insufficient -- now we also need to make sure that the
inferred effects don't provide any useful IPO information.

Example:
```julia
julia> const CONST_DICT = let d = Dict()
           for c in 'A':'z'
               push!(d, c => Int(c))
           end
           d
       end;

julia> Base.@assume_effects :total_may_throw getcharid(c) = CONST_DICT[c];

julia> @noinline callf(f, args...) = f(args...);

julia> function entry_to_be_invalidated(c)
           return callf(getcharid, c)
       end;

julia> @test Base.infer_effects((Char,)) do x
           entry_to_be_invalidated(x)
       end |> Core.Compiler.is_concrete_eval_eligible
Test Passed

julia> @test fully_eliminated(; retval=97) do
           entry_to_be_invalidated('a')
       end
Test Passed

julia> getcharid(c) = CONST_DICT[c]; # now this is not eligible for concrete evaluation

julia> @test Base.infer_effects((Char,)) do x
           entry_to_be_invalidated(x)
       end |> !Core.Compiler.is_concrete_eval_eligible
Test Failed at REPL[10]:1
  Expression: Base.infer_effects((Char,)) do x
        entry_to_be_invalidated(x)
    end |> !(Core.Compiler.is_concrete_eval_eligible)
ERROR: There was an error during testing

julia> @test !fully_eliminated() do
           entry_to_be_invalidated('a')
       end
Test Failed at REPL[11]:1
  Expression: !(fully_eliminated() do
        entry_to_be_invalidated('a')
    end)
ERROR: There was an error during testing
```
@aviatesk aviatesk added the backport 1.8 Change should be backported to release-1.8 label Apr 18, 2022
@aviatesk aviatesk requested a review from vtjnash April 18, 2022 06:24
@vtjnash vtjnash added the merge me PR is reviewed. Merge when all tests are passing label Apr 18, 2022
@DilumAluthge DilumAluthge merged commit 4c72343 into master Apr 19, 2022
@DilumAluthge DilumAluthge deleted the avi/effectfreeedge branch April 19, 2022 01:54
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Apr 19, 2022
aviatesk added a commit that referenced this pull request Apr 19, 2022
Now our abstract interpretation system caches two separate lattice
properties return type and call effects, and the previous optimization
to avoid adding backedge for a frame whose return type is `Any` turns
out to be insufficient -- now we also need to make sure that the
inferred effects don't provide any useful IPO information.

Example:
```julia
julia> const CONST_DICT = let d = Dict()
           for c in 'A':'z'
               push!(d, c => Int(c))
           end
           d
       end;

julia> Base.@assume_effects :total_may_throw getcharid(c) = CONST_DICT[c];

julia> @noinline callf(f, args...) = f(args...);

julia> function entry_to_be_invalidated(c)
           return callf(getcharid, c)
       end;

julia> @test Base.infer_effects((Char,)) do x
           entry_to_be_invalidated(x)
       end |> Core.Compiler.is_concrete_eval_eligible
Test Passed

julia> @test fully_eliminated(; retval=97) do
           entry_to_be_invalidated('a')
       end
Test Passed

julia> getcharid(c) = CONST_DICT[c]; # now this is not eligible for concrete evaluation

julia> @test Base.infer_effects((Char,)) do x
           entry_to_be_invalidated(x)
       end |> !Core.Compiler.is_concrete_eval_eligible
Test Failed at REPL[10]:1
  Expression: Base.infer_effects((Char,)) do x
        entry_to_be_invalidated(x)
    end |> !(Core.Compiler.is_concrete_eval_eligible)
ERROR: There was an error during testing

julia> @test !fully_eliminated() do
           entry_to_be_invalidated('a')
       end
Test Failed at REPL[11]:1
  Expression: !(fully_eliminated() do
        entry_to_be_invalidated('a')
    end)
ERROR: There was an error during testing
```
@aviatesk aviatesk mentioned this pull request Apr 19, 2022
67 tasks
@aviatesk aviatesk removed the backport 1.8 Change should be backported to release-1.8 label Apr 19, 2022
aviatesk added a commit to aviatesk/JET.jl that referenced this pull request Apr 19, 2022
aviatesk added a commit that referenced this pull request Apr 19, 2022
Follows up #45017.
We can relax the condition in `add_call_backedges!` for backedge optimization
by ignoring the `nonoverlayed` property when the `AbstractInterpreter`
doesn't use overlayed method table at all, since the property will never
be tainted anyway even if we add a new method later.
aviatesk added a commit that referenced this pull request Apr 20, 2022
Follows up #45017.
We can relax the condition in `add_call_backedges!` for backedge optimization
by ignoring the `nonoverlayed` property when the `AbstractInterpreter`
doesn't use overlayed method table at all, since the property will never
be tainted anyway even if we add a new method later.
aviatesk added a commit that referenced this pull request Apr 20, 2022
Follows up #45017.
We can relax the condition in `add_call_backedges!` for backedge optimization
by ignoring the `nonoverlayed` property when the `AbstractInterpreter`
doesn't use overlayed method table at all, since the property will never
be tainted anyway even if we add a new method later.
aviatesk added a commit that referenced this pull request Apr 20, 2022
Follows up #45017.
We can relax the condition in `add_call_backedges!` for backedge optimization
by ignoring the `nonoverlayed` property when the `AbstractInterpreter`
doesn't use overlayed method table at all, since the property will never
be tainted anyway even if we add a new method later.
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