Skip to content

Conversation

@c42f
Copy link
Member

@c42f c42f commented Apr 24, 2017

@Keno this is an updated version of your PR.

Resolves #137

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 69.918% when pulling 01eb1f1 on size-optimization into 4d74ac7 on master.

Copy link
Member

@andyferris andyferris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Keno @c42f could we please confirm that these changes make a positive impact on generated code given recent merged PRs? (I haven't looked but the spirit here seems ok by me).

@pure size(::Type{SA}, d::Int) where {SA <: StaticArray} = Size(SA)[d]
@pure size(::Type{<:StaticArray{S}}) where S = tuple(S.parameters...)
@inline function size(t::Type{<:StaticArray}, d::Int)
S = size(t)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is generated code better with Size(t)?

@c42f c42f force-pushed the size-optimization branch from 01eb1f1 to 26a9db2 Compare April 24, 2017 20:31
@c42f
Copy link
Member Author

c42f commented Apr 24, 2017

Yep, it's better. I didn't spend a lot of time on this, but I did check the the following using @Keno's original example:

Without patch:

julia> foo(A) = size(A,2)
julia> @code_llvm foo(ones(SMatrix{2,3}))

define i64 @julia_foo_60424(%SArray* nocapture readonly dereferenceable(48)) #0 !dbg !5 {
top:
  %1 = call i64 @julia_size_60425(%SArray* nocapture nonnull readonly %0, i64 2)
  ret i64 %1
}

With patch:

julia> foo(A) = size(A,2)
julia> @code_llvm foo(ones(SMatrix{2,3}))

define i64 @julia_foo_60360(%SArray* nocapture readonly dereferenceable(48)) #0 !dbg !5 {
top:
  ret i64 3
}

It'll be a bit of a grind, but we need to put some solid work into benchmarking StaticArrays. This kind of spot checking of the assembly is all very well, but I'm pretty sure we're going to fall into a massive performance hole sometime soon without realizing it :-/

@c42f
Copy link
Member Author

c42f commented Apr 24, 2017

I checked a few other things like bar(A) = size(A)[1] + size(A)[2] - all looks good afaict.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 70.54% when pulling 26a9db2 on size-optimization into 2a09e2c on master.

@Keno
Copy link
Contributor

Keno commented Apr 24, 2017

This looks fine to me.

@andyferris andyferris merged commit e15b6a9 into master Apr 24, 2017
@andyferris
Copy link
Member

Thanks guys!!

@SimonDanisch SimonDanisch deleted the size-optimization branch April 24, 2017 21:38
oschulz pushed a commit to oschulz/StaticArrays.jl that referenced this pull request Apr 4, 2023
* bypass constructor in getindex

* special case tuples and named tuples

* only bypass constructor for concrete types

* test on internal constructor case

* minor reorg

* define struct outside testet
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.

5 participants