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 of collect with unstable eltype #25861

Merged
merged 1 commit into from
May 15, 2018

Conversation

martinholters
Copy link
Member

As promote_typejoin(::Any, ::Any) is inferred as Any, inference of similar(dest, promote_typejoin(T, S)) is very vague as the second parameter could not only be the new element type, but also e.g. new size. A simple type-assertion improves the situation.

Example:
Before:

julia> Core.Compiler.return_type(collect, Tuple{typeof(2x for x in Real[])})
AbstractArray

After:

julia> Core.Compiler.return_type(collect, Tuple{typeof(2x for x in Real[])})
Array{_1,1} where _1

@nalimilan
Copy link
Member

Good catch. I wonder whether we could improve inference for promote_typejoin. Maybe the problem is due to the removal of the @_pure_meta annotation in join_types? If so, we could define the function differently (using a loop with @eval rather than passing the function as an argument) so that it can be marked as pure again.

@martinholters
Copy link
Member Author

I had tried renaming all existing promote_typejoin methods to _promote_typejoin and replacing the former with

promote_typejoin(@nospecialize(a), @nospecialize(b)) = _promote_typejoin(a, b)

Curiously, for an example where the generator could return either a Bool or a String, that tightened inference of intermediate results in collect from Vector to Union{Vector{Bool},Vector{String},Vector{Any}}, but the end result was then the wider AbstractArray. And I admittedly was to lazy to figure out what was going on...

@martinholters
Copy link
Member Author

martinholters commented Feb 2, 2018

But more directly to your question: No, I don't think removal of @_pure_meta would have caused this. IIUC, the number of methods to consider for promote_typejoin(::Any, ::Any) is large enough for inference to bail out anyway.

@kshyatt kshyatt added the collections Data structures holding multiple items, e.g. sets label Feb 2, 2018
@martinholters martinholters force-pushed the mh/inference_collect branch 2 times, most recently from 83961e6 to 57e7494 Compare May 8, 2018 14:11
@martinholters
Copy link
Member Author

Rebased and reworked this.

One interesting problem to inferring promote_typejoin(a,b) is illustrated by the following:

julia> foo(::Type{T}) where {T} = Union{Missing, T}
foo (generic function with 1 method)

julia> code_warntype(foo, Tuple{Any})
Variables:

Body:
  begin
      Core.SSAValue(0) = (Core.apply_type)(Main.Union, Main.Missing, $(Expr(:static_parameter, 1)))::Any
      return Core.SSAValue(0)
  end::Any

I think it should be safe to always infer Union{...} as Type, but that doesn't happen. So I've introduced a wrapper function with a type-assert.

@nalimilan nalimilan requested a review from vtjnash May 8, 2018 20:24
@vtjnash
Copy link
Member

vtjnash commented May 8, 2018

That does sound reasonably safe. It looks like currently apply_type_tfunc just instead returns Any when it gives up.

@nalimilan
Copy link
Member

I guess this means "Approved"?

Ensure that `promote_typejoin(::Any, ::Any)` is at worst inferred as
`Type`, since inference of `similar(dest, ::Any)` is very vague as the
second parameter could not only be the new element type, but also e.g.
new size. Also ensure that `_array_for(::Type, itr, ::HasShape{N})` is
inferable as an `Array{T,N}` with the correct `N`.
@martinholters martinholters force-pushed the mh/inference_collect branch from 57e7494 to 5d1480f Compare May 14, 2018 13:13
@martinholters martinholters merged commit 1b92f51 into master May 15, 2018
@martinholters martinholters deleted the mh/inference_collect branch May 15, 2018 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
collections Data structures holding multiple items, e.g. sets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants