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

inlining: make union splitting account for uncovered call #48455

Merged
merged 1 commit into from
Jan 30, 2023
Merged

Conversation

aviatesk
Copy link
Sponsor Member

#44421 changed the union-splitting to skip generating unnecessary fallback dynamic dispatch call when there is any fully covered call.
But it turned out that this is only valid when there is any fully covered call in matches for all signatures that inference split, and it is invalid if there is any union split signature against which any uncovered call is found.

Consider the following example:

# case 1
    # def
    nosplit(::Any) = [...]
    nosplit(::Int) = [...]
    # call
    nosplit(a::Any)
        split1: a::Any ┬ nosplit(a::Int)
                       └ nosplit(a::Any) # fully covers split1

# case 2
    # def
    convert(::Type{T}, ::T) = T
    # call
    convert(::Type{Union{Bool,Tuple{Int,String}}}, a::Union{Bool,Tuple{Int,Any}})
        split1: a::Bool           ─ convert(::Type{Bool}, ::Bool)                           # fully covers split1
        split2: a::Tuple{Int,Any} ─ convert(::Type{Tuple{Int,String}}, ::Tuple{Int,String}) # NOT fully covers split2

#44421 allows us to optimize the the first case, but handles the second case wrongly. This commit fixes it up while still optimizing the first case.

fix #48397.

#44421 changed the union-splitting to skip generating
unnecessary fallback dynamic dispatch call when there is any fully
covered call.
But it turned out that this is only valid when there is any fully
covered call in matches for all signatures that inference split, and it
is invalid if there is any union split signature against which any
uncovered call is found.

Consider the following example:

    # case 1
        # def
        nosplit(::Any) = [...]
        nosplit(::Int) = [...]
        # call
        nosplit(a::Any)
            split1: a::Any ┬ nosplit(a::Int)
                           └ nosplit(a::Any) # fully covers split1

    # case 2
        # def
        convert(::Type{T}, ::T) = T
        # call
        convert(::Type{Union{Bool,Tuple{Int,String}}}, a::Union{Bool,Tuple{Int,Any}})
            split1: a::Bool           ─ convert(::Type{Bool}, ::Bool)                           # fully covers split1
            split2: a::Tuple{Int,Any} ─ convert(::Type{Tuple{Int,String}}, ::Tuple{Int,String}) # NOT fully covers split2

#44421 allows us to optimize the the first case, but
handles the second case wrongly. This commit fixes it up while still
optimizing the first case.

fix #48397.
@aviatesk aviatesk added compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) backport 1.9 Change should be backported to release-1.9 labels Jan 30, 2023
@aviatesk aviatesk requested a review from Keno January 30, 2023 11:38
@aviatesk
Copy link
Sponsor Member Author

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

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

@aviatesk aviatesk merged commit 7e8515c into master Jan 30, 2023
@aviatesk aviatesk deleted the avi/48397 branch January 30, 2023 15:29
aviatesk added a commit that referenced this pull request Jan 30, 2023
#44421 changed the union-splitting to skip generating
unnecessary fallback dynamic dispatch call when there is any fully
covered call.
But it turned out that this is only valid when there is any fully
covered call in matches for all signatures that inference split, and it
is invalid if there is any union split signature against which any
uncovered call is found.

Consider the following example:

    # case 1
        # def
        nosplit(::Any) = [...]
        nosplit(::Int) = [...]
        # call
        nosplit(a::Any)
            split1: a::Any ┬ nosplit(a::Int)
                           └ nosplit(a::Any) # fully covers split1

    # case 2
        # def
        convert(::Type{T}, ::T) = T
        # call
        convert(::Type{Union{Bool,Tuple{Int,String}}}, a::Union{Bool,Tuple{Int,Any}})
            split1: a::Bool           ─ convert(::Type{Bool}, ::Bool)                           # fully covers split1
            split2: a::Tuple{Int,Any} ─ convert(::Type{Tuple{Int,String}}, ::Tuple{Int,String}) # NOT fully covers split2

#44421 allows us to optimize the the first case, but
handles the second case wrongly. This commit fixes it up while still
optimizing the first case.

fix #48397.
@aviatesk aviatesk mentioned this pull request Jan 30, 2023
35 tasks
Comment on lines +1924 to +1927
@test_throws MethodError let
Base.Experimental.@force_compile
f48397(g48397)
end
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

You can make these tests more reliable by looking at the return value here

Suggested change
@test_throws MethodError let
Base.Experimental.@force_compile
f48397(g48397)
end
let err = (@test_throws MethodError let
Base.Experimental.@force_compile
f48397(g48397)
end).value
@test err.f === f48397 && err.args === (g48397,)
end

aviatesk added a commit that referenced this pull request Jan 31, 2023
aviatesk added a commit that referenced this pull request Feb 1, 2023
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
5 participants