Add @propagate_inbounds to getindex for SMatrix #491
Conversation
src/SMatrix.jl
Outdated
|
|
||
| function getindex(v::SMatrix, i::Int) | ||
| Base.@propagate_inbounds function getindex(v::SMatrix, i::Int) | ||
| Base.@_inline_meta |
There was a problem hiding this comment.
@propagate_inbounds implies that the function is inlined AFAIK.
There was a problem hiding this comment.
ah is that true? so you dont need the inline meta?
There was a problem hiding this comment.
@propagate_inbounds
Tells the compiler to inline a function while retaining the caller's
inbounds context.
There was a problem hiding this comment.
haha, guilty! never read the docs!
There was a problem hiding this comment.
Base is full of @inline @propagate_inbounds so maybe you're not the only one
There was a problem hiding this comment.
yeah, thats were i got it from ;)
|
Thanks! Can we do this properly for all the array types? |
|
I'm not sure the inlining comments were meant as no-go for SMatrix is especially useful on the GPU, it's the only type of matrix you can use as a struct field. But I can update this for everything else too if that's a good idea. |
Nope, just wanted to point out redundant code. |
|
As in: keep the propagate inbounds, ditch the inline. |
|
My point was partly that SMatrix is only one type of SArray (does the same problem occur for SVector? Or 3D arrays?) |
|
Ok duh I missed the I'll test all types of SArray and FieldVector and update. |
|
SArrays needed FieldVectors also have Is there anything else relevant to the (immutable) GPU context? |
|
Also I haven't added any tests for this because of the need for a GPU. Is it possible to test GPUArrays integration with JLArray? @SimonDanisch ? |
|
I mean, all of this isn't GPU specific! You will want to have inbounds propagating in all performance sensitive code. Not sure how to test this. A hacky way would be to search for the throw in the IR :P |
|
Not sure I can manage that kind of hack! And sure its useful for other things as well, but it doesn't break them entirely without it! I just meant having some test to ensure |
Needed to work with GPUArrays. See: JuliaGPU/GPUArrays.jl#157