StaticArraysCore.jl icon indicating copy to clipboard operation
StaticArraysCore.jl copied to clipboard

Use `@assume_effects` instead of `@pure` for Julia >v1.8

Open thchr opened this issue 3 years ago • 8 comments

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.

thchr avatar Jun 07 '22 16:06 thchr

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 data Powered by Codecov. Last update 4ced4ff...d676b70. Read the comment docs.

codecov[bot] avatar Jun 07 '22 17:06 codecov[bot]

Good point but I'd wait until 1.8-rc2 is released before merging this.

mateuszbaran avatar Jun 07 '22 17:06 mateuszbaran

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.

LilithHafner avatar Jul 15 '22 12:07 LilithHafner

Since we can remove the annotations fully, we can do this without waiting, right?

oscardssmith avatar Sep 21 '22 20:09 oscardssmith

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.

thchr avatar Sep 21 '22 20:09 thchr

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.

oscardssmith avatar Sep 21 '22 21:09 oscardssmith

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.

thchr avatar Sep 21 '22 21:09 thchr

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.

oscardssmith avatar Sep 21 '22 21:09 oscardssmith