-
-
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
Improve performance annotation docs #17722
Conversation
@@ -1114,6 +1114,54 @@ properties: | |||
LLVM auto-vectorization may kick in automatically, leading to no further | |||
speedup with :obj:`@simd`. | |||
|
|||
While :obj:`@simd` needs to be placed in front of a loop, that is not the case | |||
for :obj:`@inbounds`. It can be placed in front of any expression as long as it | |||
is inside a function, the only place where it could be outside of a function |
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.
Run on sentence here. Turn the comma into a period, I think.
Should I improve anything else? |
in getindex(::Array{Int64,1}, ::Int64) at ./array.jl:309 | ||
in eval(::Module, ::Any) at ./boot.jl:225 | ||
in macro expansion at ./REPL.jl:92 [inlined] | ||
in (::Base.REPL.##1#2{Base.REPL.REPLBackend})() at ./event.jl:46 |
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.
did you run this on 0.5? the backtraces should look different now
Nope, completely forgot about that after switching branch. Will do that right away |
Previous error message was 0.4
Backtrace modified |
for :obj:`@inbounds`. It can be placed in front of any expression as long as it | ||
is inside a function. The only place where it can be outside of a function | ||
definition is in front of a loop. | ||
:: |
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.
This double-colon is malformed RST I think, see the sphinx errors
/home/Tony/julia/doc/manual/performance-tips.rst:1122: ERROR: Unexpected indentation.
/home/Tony/julia/doc/manual/performance-tips.rst:1146: ERROR: Unexpected indentation.
I can only imagine putting one more newline between |
You have to scroll up to the start of the output, there are a lot of harmless warnings and the actual malformed rst errors are buried at the start of those. |
It is now fixed, thanks for the help. |
This will fix #13234 as well, right? |
Yes, I mention it doesn't work on whole functions and a workaround must be done. |
@eschnett @blakejohnson does this look all good to you? |
While :obj:`@simd` needs to be placed in front of a loop, that is not the case | ||
for :obj:`@inbounds`. It can be placed in front of any expression as long as it | ||
is inside a function. The only place where it can be outside of a function | ||
definition is in front of a loop. |
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.
This information (outside functions only in front of loops) is news to me. Does this mean that a stand-alone statement
@inbounds a[1] = 0
is malformed?
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.
It is not. It just may or may not disable boundscheck depending on many other compiler heuristics.
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.
FWIW, The only place where it can be outside of a function definition is in front of a loop
is very misleading....
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.
What should it say? What does malformed mean in programming btw?
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.
Well, it is also just not true. At global scope it is perfectly fine to write:
x = 1
@inbounds x
But whereas without @inbounds
this would print 1
, with @inbounds
it doesn't print anything. However, I think the inbounds meta information is ignored outside of a function.
I would simplify the description to say that @inbounds
can be written in front of any expression, and in some expressions this may be useful to limit the scope of what you are declaring to be "in bounds".
Would this be alright?
|
How about "it will generally have an effect when used inside a function definition"? |
is inside a function. The only place where it can be outside of a function | ||
definition is in front of a loop. | ||
for :obj:`@inbounds`. It can be placed in front of any expression, however | ||
the compiler isn't always capable of recognizing where to remove bounds check, |
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.
Bit of a run-on. Just end this sentence here ("where to remove bounds checks.") and have "Limiting" start a new one.
What is the backport pending label for? |
@lopezm94 could you give me commit access to your branch, following the instructions here: https://help.github.com/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork/ I'd like to make a more more edits and then get this merged. |
Permission given just now @blakejohnson |
I wonder if we want to use some of this stuff, or close this. @chriselrod Could you help decide, if you have a moment? |
Looking over the PR, it looks like it's mostly about where you can apply the macros?
While I think it's better to limit the scope of |
I think the docstrings cover this, and if not we should add it there instead. This PR is for the old docsystem and lots have changed so I will just close this. |
Continuation of pull request #17700