fix some invalidations when loading Static.jl #46553
Conversation
…ray)` This improvement prevents some invalidations in abstractarray.jl when loading Static.jl.
|
But it looks like it does not at the moment, at least not on Julia v1.8. Is it already fixed on |
|
I would try to fix this at a higher level; presumably this fails to infer only because the array type is being inferred abstractly. |
Yes, the first argument of |
…stractArray)`" This reverts commit 6b3a3ae.
This prevents some invalidations in `mightalias(A::AbstractArray, B::AbstractArray)` in abstractarray.jl when loading Static.jl.
ranocha
changed the title
mightalias(A::AbstractArray, B::AbstractArray)
|
The last two cases reported above come from The first case (with 10 children) is reduced to 7 children: I could not really find a good place where the type instability happens there since |
base/array.jl
Outdated
| @@ -2325,7 +2325,7 @@ findall(testf::Function, A) = collect(first(p) for p in pairs(A) if testf(last(p | |||
|
|
|||
| # Broadcasting is much faster for small testf, and computing | |||
| # integer indices from logical index using findall has a negligible cost | |||
| findall(testf::Function, A::AbstractArray) = findall(testf.(A)) | |||
| findall(testf::Function, A::AbstractArray) = findall(map(testf, A)) | |||
There was a problem hiding this comment.
This would increase the allocation size as map return a Array{Bool} by default while broadcast return a BitArray.
There was a problem hiding this comment.
Okay, didn't know that. Thanks for noticing!
This prevents some invalidations in `mightalias(A::AbstractArray, B::AbstractArray)` in abstractarray.jl when loading Static.jl.
Here we specialize on the function instead of using map since broadcasting returns a BitArray, map a Vector{Bool}.
| @@ -2325,7 +2325,7 @@ findall(testf::Function, A) = collect(first(p) for p in pairs(A) if testf(last(p | |||
|
|
|||
| # Broadcasting is much faster for small testf, and computing | |||
| # integer indices from logical index using findall has a negligible cost | |||
| findall(testf::Function, A::AbstractArray) = findall(testf.(A)) | |||
| findall(testf::F, A::AbstractArray) where {F<:Function} = findall(testf.(A)) | |||
There was a problem hiding this comment.
This is another possibility to fix the invalidations coming from this by specializing on testf since map has also disadvantages, see #46553 (comment). However, always specializing could also have disadvantages in terms of latency, of course. I don't know what's best here 🤷
There are basically four options I see
- My first suggestion adding a type assertion in
mightalias - The version with
maphere, increasing allocations - The version with specialization here, possibly increasing compiler workload
- Just do nothing for now and hope that the compiler will be improved later
|
@aviatesk Could we get Conditional to refine this |
|
I suspect this could deserve a long-ish conversation. I've often wondered about making this change (mostly in Previously, my inclination was to wait until precompilation got better, because good precompilation circumvents the latency. But we may have basically arrived there, or will soon once we can cache native code. So maybe the time is ripe for this conversation. There are other issues where we might consider changing our verdict, e.g., #36454. So I guess the key question is, are there reasonable benchmarks we could use to help make this decision? |
I don't think I have really good benchmarks for this. I could of course provide some biased benchmarks from our packages such as Trixi.jl, but we will need more diverse input, I think. In the end, there is no single best benchmark for everybody but we should look at something reasonable for most people. |
Currently our inference isn't able to propagate `isa`-based type
constraint for cases like `isa(Type{<:...}, DataType)` since
`typeintersect` may return `Type` object itself when taking `Type`
object and `iskindtype`-object.
This case happens in the following kind of situation (motivated by the
discussion at <#46553 (comment)>):
```julia
julia> function isa_kindtype(T::Type{<:AbstractVector})
if isa(T, DataType)
# `T` here should be inferred as `DataType` rather than `Type{<:AbstractVector}`
return T.name.name # should be inferred as ::Symbol
end
return nothing
end
isa_kindtype (generic function with 1 method)
julia> only(code_typed(isa_kindtype; optimize=false))
CodeInfo(
1 ─ %1 = (T isa Main.DataType)::Bool
└── goto #3 if not %1
2 ─ %3 = Base.getproperty(T, :name)::Any
│ %4 = Base.getproperty(%3, :name)::Any
└── return %4
3 ─ return Main.nothing
) => Any
```
This commit improves the situation by adding a special casing for
abstract interpretation, rather than changing the behavior of
`typeintersect`.
Currently our inference isn't able to propagate `isa`-based type
constraint for cases like `isa(Type{<:...}, DataType)` since
`typeintersect` may return `Type` object itself when taking `Type`
object and `iskindtype`-object.
This case happens in the following kind of situation (motivated by the
discussion at <#46553 (comment)>):
```julia
julia> function isa_kindtype(T::Type{<:AbstractVector})
if isa(T, DataType)
# `T` here should be inferred as `DataType` rather than `Type{<:AbstractVector}`
return T.name.name # should be inferred as ::Symbol
end
return nothing
end
isa_kindtype (generic function with 1 method)
julia> only(code_typed(isa_kindtype; optimize=false))
CodeInfo(
1 ─ %1 = (T isa Main.DataType)::Bool
└── goto #3 if not %1
2 ─ %3 = Base.getproperty(T, :name)::Any
│ %4 = Base.getproperty(%3, :name)::Any
└── return %4
3 ─ return Main.nothing
) => Any
```
This commit improves the situation by adding a special casing for
abstract interpretation, rather than changing the behavior of
`typeintersect`.
…cts (#46712) Currently our inference isn't able to propagate `isa`-based type constraint for cases like `isa(Type{<:...}, DataType)` since `typeintersect` may return `Type` object itself when taking `Type` object and `iskindtype`-object. This case happens in the following kind of situation (motivated by the discussion at <#46553 (comment)>): ```julia julia> function isa_kindtype(T::Type{<:AbstractVector}) if isa(T, DataType) # `T` here should be inferred as `DataType` rather than `Type{<:AbstractVector}` return T.name.name # should be inferred as ::Symbol end return nothing end isa_kindtype (generic function with 1 method) julia> only(code_typed(isa_kindtype; optimize=false)) CodeInfo( 1 ─ %1 = (T isa Main.DataType)::Bool └── goto #3 if not %1 2 ─ %3 = Base.getproperty(T, :name)::Any │ %4 = Base.getproperty(%3, :name)::Any └── return %4 3 ─ return Main.nothing ) => Any ``` This commit improves the situation by adding a special casing for abstract interpretation, rather than changing the behavior of `typeintersect`.
…46553) This prevents some invalidations in `mightalias(A::AbstractArray, B::AbstractArray)` in abstractarray.jl when loading Static.jl. Here we specialize on the function instead of using map since `broadcasting` returns a BitArray, `map` returns a Vector{Bool}.
ranocha
added
backport 1.8
This prevents some invalidations in `mightalias(A::AbstractArray, B::AbstractArray)`
in abstractarray.jl when loading Static.jl.
Here we specialize on the function instead of using map since
`broadcasting` returns a BitArray, `map` returns a Vector{Bool}.
(cherry picked from commit 31d4c22)
This improvement prevents some invalidations in abstractarray.jl when loading Static.jl.
This is based on the following code:
I suggest the labels
latencyandbackport-1.8.