Skip to content
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

at-views macro to convert a whole block of code to slices=views #20164

Merged
merged 10 commits into from
Jan 24, 2017

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Jan 20, 2017

This PR provides an @views macro 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 @view or calls to view everywhere. This is especially easier for updating operations like .+=, where you can't put view on the left-hand side (as discussed on discourse).

@stevengj stevengj added broadcast Applying a function over a collection needs tests Unit tests are required for this change labels Jan 20, 2017
# 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)...)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

replace_ref_end!?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch, thanks.

@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:
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

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.

@stevengj stevengj removed the needs tests Unit tests are required for this change label Jan 21, 2017
@stevengj stevengj mentioned this pull request Jan 21, 2017
27 tasks
# 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...)
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need an array-of-arrays special case like dotview has?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because for scalar indexing it already gives getindex

@stevengj
Copy link
Member Author

Okay to merge? @StefanKarpinski, any thoughts on this?

@stevengj
Copy link
Member Author

Since it seems everyone likes this, I'll merge in a day or so if there are no further objections.

@stevengj stevengj merged commit d87239c into JuliaLang:master Jan 24, 2017
@stevengj stevengj deleted the views branch January 24, 2017 15:51
@StefanKarpinski
Copy link
Sponsor Member

Sorry, I was traveling but I do have one thought: why not just fold this into the existing @view macro? I.e. if you write @view A[1,2:end-1,:] that will work but you can also write

@view begin
     # in here all indexing expressions create views
end

Grammatically, having @view and @views reads better, but it seems somewhat unnecessary to have both of these macros when they do such similar things.

@stevengj
Copy link
Member Author

Well, they aren't quite the same. @views A[B[1:10]] will use views for both the A and B slices, but @view A[B[1:10]] will only use a view for the A slice.

That being said, I suspect that @views is much more useful, so we might want to simply deprecate @view?

@mbauman
Copy link
Sponsor Member

mbauman commented Jan 25, 2017

They (also) behave differently in the scalar case. @view will always return a view (including a 0-dimensional one), whereas @views will return scalars for scalar indexing. I had the same thought initially, but I think the @views behavior is generally more useful and it'd be breaking to change @view's scalar behaviors.

@StefanKarpinski
Copy link
Sponsor Member

It seems like having both @view and @views makes a fair bit of sense. The differences should be well documented so that people know why both exist despite similarities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
broadcast Applying a function over a collection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants