-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
9034aff
at-views macro to convert a whole block of code to slices=views
stevengj 60ba3dd
inlining seems to eliminate the splatting penalty in maybeview(A, arg…
stevengj f5f1098
handle :end in at-views
stevengj c9e0386
added at-views tests
stevengj 0fa9181
NEWS for at-views
stevengj 03bd1c6
code simplification
stevengj 416b338
doc tweak
stevengj 8052259
doc tweak
stevengj dccb16b
added manual ref and performance tip on views
stevengj b7f8da6
typo
stevengj File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1384,6 +1384,7 @@ export | |
@label, | ||
@goto, | ||
@view, | ||
@views, | ||
|
||
# SparseArrays module re-exports | ||
SparseArrays, | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -750,3 +750,66 @@ kwdef_val(::Type{Cwstring}) = Cwstring(C_NULL) | |
kwdef_val{T<:Integer}(::Type{T}) = zero(T) | ||
|
||
kwdef_val{T}(::Type{T}) = T() | ||
|
||
############################################################################ | ||
# @views macro (not defined in subarray.jl because of a bootstrapping | ||
# issue with the code generation below). | ||
|
||
# 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...) | ||
@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: | ||
let pi(expr) = :(@propagate_inbounds $expr) | ||
for nargs = 1:5 | ||
args = Symbol[Symbol("x",i) for i = 1:nargs] | ||
numargs = Expr[:($(Symbol("x",i))::Number) for i = 1:nargs] | ||
eval(pi(Expr(:(=), Expr(:call, :maybeview, :A, args...), | ||
Expr(:block, Expr(:call, :getindex, :A, args...))))) | ||
eval(pi(Expr(:(=), Expr(:call, :maybeview, :(A::AbstractArray), args...), | ||
Expr(:block, Expr(:call, :view, :A, args...))))) | ||
eval(pi(Expr(:(=), Expr(:call, :maybeview, :(A::AbstractArray), numargs...), | ||
Expr(:block, Expr(:call, :getindex, :A, args...))))) | ||
end | ||
end | ||
|
||
_views(x) = x | ||
_views(x::Symbol) = esc(x) | ||
function _views(ex::Expr) | ||
if ex.head in (:(=), :(.=)) | ||
# 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)...) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, thanks. |
||
else | ||
h = string(ex.head) | ||
if last(h) == '=' | ||
# don't use view on the lhs of an op-assignment | ||
Expr(first(h) == '.' ? :(.=) : :(=), esc(ex.args[1]), | ||
Expr(:call, esc(Symbol(h[1:end-1])), _views(ex.args[1]), | ||
map(_views, ex.args[2:end])...)) | ||
else | ||
Expr(ex.head, map(_views, ex.args)...) | ||
end | ||
end | ||
end | ||
|
||
""" | ||
@views code | ||
|
||
Convert every array-slicing operation in the given `code` | ||
(which may be a `begin`/`end` block, loop, function, etc.) | ||
to return a view. Scalar indices, non-array types, and | ||
explicit `getindex` calls (as opposed to `array[...]`) are | ||
unaffected. | ||
|
||
Note that the `@views` macro only affects `array[...]` expressions | ||
that appear explicitly in the given code, not array slicing that | ||
occurs in functions called by the code. | ||
""" | ||
macro views(x) | ||
_views(x) | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
and it wasn't getting inlined, but I guess I should try again with
@propagate_inbounds
?There was a problem hiding this comment.
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)
, butfoo() = g(3) + g(4) - g(6,7) + g(8)
is definitely inliningg
(and simplifies toret i64 1
).Great, that will simply things. The
dotview
function inbroadcast.jl
can be similarly simplified.There was a problem hiding this comment.
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.