StaticArraysCore.jl
StaticArraysCore.jl copied to clipboard
Use `@assume_effects` instead of `@pure` for Julia >v1.8
Since :total_may_throw was renamed to :foldable only recently (cf. https://github.com/JuliaLang/julia/pull/45534), it seems the cut-off would be an as-yet unreleased v1.8.0-rc2.
Codecov Report
Merging #1 (d676b70) into main (4ced4ff) will not change coverage. The diff coverage is
100.00%.
@@ Coverage Diff @@
## main #1 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 1 1
Lines 46 46
=========================================
Hits 46 46
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/StaticArraysCore.jl | 100.00% <100.00%> (ø) |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 4ced4ff...d676b70. Read the comment docs.
Good point but I'd wait until 1.8-rc2 is released before merging this.
I think it makes more sense to remove these annotations entirely. In 1.7.3, and presumably all later versions of Julia, these functions already fold without annotations
REPL session
julia> tuple_length(T::Type{<:Tuple}) = length(T.parameters)
tuple_length (generic function with 1 method)
julia> tuple_length(T::Tuple) = length(T)
tuple_length (generic function with 2 methods)
julia> tuple_prod(T::Type{<:Tuple}) = length(T.parameters) == 0 ? 1 : *(T.parameters...)
tuple_prod (generic function with 1 method)
julia> tuple_prod(T::Tuple) = prod(T)
tuple_prod (generic function with 2 methods)
julia> tuple_minimum(T::Type{<:Tuple}) = length(T.parameters) == 0 ? 0 : minimum(tuple(T.parameters...))
tuple_minimum (generic function with 1 method)
julia> tuple_minimum(T::Tuple) = minimum(T)
tuple_minimum (generic function with 2 methods)
julia> code_typed() do
tuple_length(Tuple{Int, Float32})
end
1-element Vector{Any}:
CodeInfo(
1 ─ return 2
) => Int64
julia> code_typed() do
tuple_length((17, 29))
end
1-element Vector{Any}:
CodeInfo(
1 ─ return 2
) => Int64
julia> code_typed() do
tuple_prod((17, 29))
end
1-element Vector{Any}:
CodeInfo(
1 ─ return 493
) => Int64
julia> code_typed() do
tuple_prod(Tuple{17, 29})
end
1-element Vector{Any}:
CodeInfo(
1 ─ return 493
) => Int64
julia> code_typed() do
tuple_minimum(Tuple{})
end
1-element Vector{Any}:
CodeInfo(
1 ─ return 0
) => Int64
julia> code_typed() do
tuple_minimum((4,))
end
1-element Vector{Any}:
CodeInfo(
1 ─ return 4
) => Int64
julia> versioninfo()
Julia Version 1.7.3
Commit 742b9abb4d (2022-05-06 12:58 UTC)
Platform Info:
OS: macOS (x86_64-apple-darwin21.4.0)
CPU: Intel(R) Core(TM) i5-8210Y CPU @ 1.60GHz
WORD_SIZE: 64
LIBM: libopenlibm
LLVM: libLLVM-12.0.1 (ORCJIT, skylake)
Further, these annotations are not sound as written. For example, tuple_prod calls prod on an arbitrary tuple which in turn calls * on arbitrary types. This is not always foldable.
Since we can remove the annotations fully, we can do this without waiting, right?
The main reason I haven't followed up on @LilithHafner's point is that I didn't really have the drive to check whether the @pure calls in fact really have no effect or not now (similarly, not to figure out which ones are sound or not). If anyone has the drive to do that, it'd be great - this was just a simple replacement of the @pure calls, assuming that @assume_effects would at least be an improvement over that.
I've done the testing, and unfortunately this isn't actually getting marked as consistent. That said, it is cheap enough to inline, so it does constant propagate anyway.
So are you suggesting we might as well just go with the @assume_effects changes here, and then further clean-up/fixing of effects/removal of specific unnecessary annotations could be done later on?
That would be my inclination, personally.
I think (although it should be sanity checks) that we can just remove the annotations entirely. Effect analysis won't figure it out, but constant prop will. Getting constant prop working is probably a good idea, but shouldn't change the performance here.