Skip to content

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

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

thchr
Copy link
Collaborator

@thchr thchr commented Jun 7, 2022

Since :total_may_throw was renamed to :foldable only recently (cf. JuliaLang/julia#45534), it seems the cut-off would be an as-yet unreleased v1.8.0-rc2.

@codecov
Copy link

codecov bot commented Jun 7, 2022

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.

@mateuszbaran
Copy link
Collaborator

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

@LilithHafner
Copy link

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(
1return 2
) => Int64

julia> code_typed() do 
           tuple_length((17, 29))
       end
1-element Vector{Any}:
 CodeInfo(
1return 2
) => Int64

julia> code_typed() do 
           tuple_prod((17, 29))
       end
1-element Vector{Any}:
 CodeInfo(
1return 493
) => Int64

julia> code_typed() do 
           tuple_prod(Tuple{17, 29})
       end
1-element Vector{Any}:
 CodeInfo(
1return 493
) => Int64

julia> code_typed() do 
           tuple_minimum(Tuple{})
       end
1-element Vector{Any}:
 CodeInfo(
1return 0
) => Int64

julia> code_typed() do 
           tuple_minimum((4,))
       end
1-element Vector{Any}:
 CodeInfo(
1return 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.

@oscardssmith
Copy link
Member

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

@thchr
Copy link
Collaborator Author

thchr commented Sep 21, 2022

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.

@oscardssmith
Copy link
Member

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.

@thchr
Copy link
Collaborator Author

thchr commented Sep 21, 2022

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.

@oscardssmith
Copy link
Member

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.

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.

4 participants