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

Improve inference in typejoin #44390

Merged
merged 3 commits into from
Mar 3, 2022
Merged

Improve inference in typejoin #44390

merged 3 commits into from
Mar 3, 2022

Conversation

timholy
Copy link
Member

@timholy timholy commented Mar 1, 2022

This fixes hundreds of promote_rule invalidations from Unitful. Discovered in a conversation with @louisponet

@aviatesk, inference of typejoin in one case shows an interesting phenomenon:

descend(typejoin, (Type{Int}, Any); optimize=false, iswarn=true)

reveals that inference fails to discover that a.name returns a TypeName and a.parameters returns a SimpleVector. AFAICT the Const annotation interferes with knowing that a behaves as a DataType. Is this easily fixable or should I add some annotations?

This fixes hundreds of invalidations from Unitful.
@aviatesk
Copy link
Member

aviatesk commented Mar 1, 2022

Reduced a bit further:

julia> code_typed((Type{Int},); optimize=false) do x
           x = x::DataType
           while x <: Number
               x = supertype(x)::DataType
           end
           x.name
       end
1-element Vector{Any}:
 CodeInfo(
1 ─      (x@_3 = x@_2)::Core.Const(Int64)
└──      (x@_3 = Core.typeassert(x@_3::Core.Const(Int64), Main.DataType))::Core.Const(Int64)
2%3 = (x@_3 <: Main.Number)::Bool
└──      goto #4 if not %3
3%5 = Main.supertype(x@_3)::Any
│        (x@_3 = Core.typeassert(%5, Main.DataType))::DataType
└──      goto #2
4%8 = Base.getproperty(x@_3, :name)::Any
└──      return %8
) => Any

@aviatesk aviatesk self-assigned this Mar 1, 2022
@aviatesk
Copy link
Member

aviatesk commented Mar 1, 2022

Okay, so I figured out one of the problems here is that supertype(T) can produce constant information when T is a precise Type object and so inference on

while [...]
    x = supertype(x)
end

requires data flow iterations until the type of x gets converged.
And when we try to converge x here, we will end up with a looser type when the initial type of x is precise (e.g. x::Const(Int)):

julia> CC.tmerge(
           Union{Type{Int},Type{Integer},Type{Signed}},
           Union{Type{Integer},Type{Signed},Type{Real}}
           )
Type

Still I'm not sure why (x@_3 = Core.typeassert(%5, Main.DataType))::DataType doesn't "recover" x@_3::DataType information.


So the following uses of inferencebarrier improves the inference of typejoin (and it also cuts off the need for data flow iteration explained above):

diff --git a/base/promotion.jl b/base/promotion.jl
index 845e16ca499..4355a9d3364 100644
--- a/base/promotion.jl
+++ b/base/promotion.jl
@@ -77,7 +77,7 @@ function typejoin(@nospecialize(a), @nospecialize(b))
     elseif b <: Tuple
         return Any
     end
-    a, b = a::DataType, b::DataType
+    a, b = inferencebarrier(a)::DataType, inferencebarrier(b)::DataType
     while b !== Any
         if a <: b.name.wrapper
             while a.name !== b.name

Maybe we want to file this problem in a separate issue?
/cc @vtjnash @martinholters

@timholy
Copy link
Member Author

timholy commented Mar 1, 2022

Maybe we want to file this problem in a separate issue?

Sure, do you want to open it? You've clearly taken it farther than me.

Should I add the inferencebarrier to this PR, or would you rather leave that out pending possible changes to inference? Update: I went ahead and added it here.

base/promotion.jl Outdated Show resolved Hide resolved
@aviatesk
Copy link
Member

aviatesk commented Mar 3, 2022

Sure, do you want to open it?

Done: #44425

Should I add the inferencebarrier to this PR

yep, the apparent allocation with inferencebarrier should be removed by our SROA pass, and so that change won't incur any runtime performance problem.

@Keno
Copy link
Member

Keno commented Mar 3, 2022

julia> CC.tmerge(
           Union{Type{Int},Type{Integer},Type{Signed}},
           Union{Type{Integer},Type{Signed},Type{Real}}
           )

Seems like tmerge could give DataType here.

@aviatesk aviatesk added the merge me PR is reviewed. Merge when all tests are passing label Mar 3, 2022
@timholy timholy merged commit 9c1572d into master Mar 3, 2022
@timholy timholy deleted the teh/typejoin_inference branch March 3, 2022 15:22
@timholy timholy removed the merge me PR is reviewed. Merge when all tests are passing label Mar 3, 2022
@timholy timholy added the backport 1.8 Change should be backported to release-1.8 label Jun 22, 2022
@timholy
Copy link
Member Author

timholy commented Jun 22, 2022

Rats, I should have tagged this for backporting a long time ago. Fixes about 1800 invalidations from loading Makie. CC @SimonDanisch.

aviatesk pushed a commit that referenced this pull request Jun 22, 2022
This fixes hundreds of invalidations from Unitful.

Co-authored by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
@aviatesk
Copy link
Member

Backported.

@aviatesk aviatesk removed the backport 1.8 Change should be backported to release-1.8 label Jun 22, 2022
@aviatesk aviatesk mentioned this pull request Jun 22, 2022
36 tasks
KristofferC pushed a commit that referenced this pull request Jul 4, 2022
This fixes hundreds of invalidations from Unitful.

Co-authored by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
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.

4 participants