at-views macro to convert a whole block of code to slices=views#20164
at-views macro to convert a whole block of code to slices=views#20164stevengj merged 10 commits intoJuliaLang:masterfrom
Conversation
stevengj
added
broadcast
base/util.jl
Outdated
| # don't use view on the lhs of an assignment | ||
| Expr(ex.head, esc(ex.args[1]), _views(ex.args[2])) | ||
| elseif ex.head == :ref | ||
| Expr(:call, :maybeview, map(_views, ex.args)...) |
base/util.jl
Outdated
| @propagate_inbounds maybeview(A::AbstractArray, args::Number...) = getindex(A, args...) | ||
| @propagate_inbounds maybeview(A) = getindex(A) | ||
| @propagate_inbounds maybeview(A::AbstractArray) = getindex(A) | ||
| # avoid splatting penalty in common cases: |
There was a problem hiding this comment.
Was this really necessary in your testing? I'd be surprised if the @propagate_inbounds definitions above have a splatting penalty since they get inlined.
There was a problem hiding this comment.
I tried a simple test case
f(x) = 1
f(x,y) = 2
g(x...) = f(x...)
and it wasn't getting inlined, but I guess I should try again with @propagate_inbounds?
There was a problem hiding this comment.
Hmm, I think I was getting deceived: @code_llvm g(3) looks more complicated than @code_llvm f(3), but foo() = g(3) + g(4) - g(6,7) + g(8) is definitely inlining g (and simplifies to ret i64 1).
Great, that will simply things. The dotview function in broadcast.jl can be similarly simplified.
There was a problem hiding this comment.
Yeah, @code_llvm can be deceiving with splatted @inline functions because it shows the LLVM function for them... but that's not at all what happens when they get inlined into another function. I almost always will define simple fixed-argument wrapper functions when I'm trying to test these guys.
…s...) = getindex(A, args...), and similarly for dotview
| # maybeview is like getindex, but returns a view for slicing operations | ||
| # (while remaining equivalent to getindex for scalar indices and non-array types) | ||
| @propagate_inbounds maybeview(A, args...) = getindex(A, args...) | ||
| @propagate_inbounds maybeview(A::AbstractArray, args...) = view(A, args...) |
There was a problem hiding this comment.
does this need an array-of-arrays special case like dotview has?
There was a problem hiding this comment.
No, because for scalar indexing it already gives getindex
|
Okay to merge? @StefanKarpinski, any thoughts on this? |
|
Since it seems everyone likes this, I'll merge in a day or so if there are no further objections. |
|
Sorry, I was traveling but I do have one thought: why not just fold this into the existing @view begin
# in here all indexing expressions create views
endGrammatically, having |
|
Well, they aren't quite the same. That being said, I suspect that |
|
They (also) behave differently in the scalar case. |
|
It seems like having both |
4 participants
This PR provides an
@viewsmacro that you can apply to a whole block of code (e.g. a whole function definition) to convert every array-slicing operation into a view. Scalar indexing is not affected.This allows you to opt-in to slices=views with minimal effort, a lot easier than adding
@viewor calls tovieweverywhere. This is especially easier for updating operations like.+=, where you can't putviewon the left-hand side (as discussed on discourse).