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 cat inferrability #45028

Merged
merged 1 commit into from
Apr 25, 2022
Merged

improve cat inferrability #45028

merged 1 commit into from
Apr 25, 2022

Conversation

aviatesk
Copy link
Member

Make cat inferrable even if its arguments are not fully constant:

julia> r = rand(Float32, 56, 56, 64, 1);

julia> f(r) = cat(r, r, dims=(3,))
f (generic function with 1 method)

julia> @inferred f(r);

julia> last(@code_typed f(r))
Array{Float32, 4}

After descending into its call graph, I found that constant propagation
is prohibited at cat_t(::Type{T}, X...; dims) due to the method instance
heuristic, i.e. its body is considered to be too complex for successful
inlining although it's explicitly annotated as @inline.
But for this case, the constant propagation is greatly helpful both for
abstract interpretation and optimization since it can improve the return
type inference.

Since it is not an easy task to improve the method instance heuristic,
which is our primary logic for constant propagation, this commit
does a quick fix by helping inference with the @constprop annotation.

There is another issue that tcurrently there is no good way to properly apply
@constprop/@inline effects to a keyword function (as a note, this
is a general issue of macro annotations on a method definition).
So this commit also changes some internal helper functions for cat so
that now they are not keyword ones: these changes are also necessary
for the @inline annotation on cat_t to be effective to trick
the method instance heuristic.

@aviatesk aviatesk requested a review from JeffBezanson April 19, 2022 08:33
@aviatesk aviatesk force-pushed the avi/infcat branch 2 times, most recently from bf99cde to 10f787a Compare April 20, 2022 05:35
aviatesk added a commit to aviatesk/SparseArrays.jl that referenced this pull request Apr 20, 2022
After JuliaLang/julia#45028, it will be more recommended to use this
more type stable version of `cat_t`.
Make `cat` inferrable even if its arguments are not fully constant:
```julia
julia> r = rand(Float32, 56, 56, 64, 1);

julia> f(r) = cat(r, r, dims=(3,))
f (generic function with 1 method)

julia> @inferred f(r);

julia> last(@code_typed f(r))
Array{Float32, 4}
```

After descending into its call graph, I found that constant propagation
is prohibited at `cat_t(::Type{T}, X...; dims)` due to the method instance
heuristic, i.e. its body is considered to be too complex for successful
inlining although it's explicitly annotated as `@inline`.
But for this case, the constant propagation is greatly helpful both for
abstract interpretation and optimization since it can improve the return
type inference.

Since it is not an easy task to improve the method instance heuristic,
which is our primary logic for constant propagation, this commit does
a quick fix by helping inference with the `@constprop` annotation.

There is another issue that currently there is no good way to properly
apply `@constprop`/`@inline` effects to a keyword function (as a note,
this is a general issue of macro annotations on a method definition).
So this commit also changes some internal helper functions of `cat`
so that now they are not keyword ones: the changes are also necessary
for the `@inline` annotation on `cat_t` to be effective to trick
the method instance heuristic.
aviatesk added a commit to aviatesk/SparseArrays.jl that referenced this pull request Apr 20, 2022
After JuliaLang/julia#45028, it will be more recommended to use this
more type stable version of `cat_t`.
@aviatesk aviatesk merged commit 65b9be4 into master Apr 25, 2022
@aviatesk aviatesk deleted the avi/infcat branch April 25, 2022 02:50
aviatesk added a commit to aviatesk/SparseArrays.jl that referenced this pull request Apr 26, 2022
After JuliaLang/julia#45028, it will be more recommended to use this
more type stable version of `cat_t`.
@kpamnany kpamnany added the backport 1.8 Change should be backported to release-1.8 label Jul 21, 2022
KristofferC pushed a commit that referenced this pull request Aug 6, 2022
Make `cat` inferrable even if its arguments are not fully constant:
```julia
julia> r = rand(Float32, 56, 56, 64, 1);

julia> f(r) = cat(r, r, dims=(3,))
f (generic function with 1 method)

julia> @inferred f(r);

julia> last(@code_typed f(r))
Array{Float32, 4}
```

After descending into its call graph, I found that constant propagation
is prohibited at `cat_t(::Type{T}, X...; dims)` due to the method instance
heuristic, i.e. its body is considered to be too complex for successful
inlining although it's explicitly annotated as `@inline`.
But for this case, the constant propagation is greatly helpful both for
abstract interpretation and optimization since it can improve the return
type inference.

Since it is not an easy task to improve the method instance heuristic,
which is our primary logic for constant propagation, this commit does
a quick fix by helping inference with the `@constprop` annotation.

There is another issue that currently there is no good way to properly
apply `@constprop`/`@inline` effects to a keyword function (as a note,
this is a general issue of macro annotations on a method definition).
So this commit also changes some internal helper functions of `cat`
so that now they are not keyword ones: the changes are also necessary
for the `@inline` annotation on `cat_t` to be effective to trick
the method instance heuristic.

(cherry picked from commit 65b9be4)
@KristofferC KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Aug 7, 2022
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