DOC: Improve SIMD documentation(1/4) #19581
Conversation
seiko2plus
added
component: documentation
component: SIMD
seiko2plus
force-pushed
the
improve_simd_doc
branch
2 times, most recently
from
9936de3
to
296b5dc
Compare
seiko2plus
force-pushed
the
improve_simd_doc
branch
from
296b5dc
to
ecb6090
Compare
|
@seiko2plus this is still draft, however there is valuable content in here already. Should we perhaps review and merge this as is, and leave the TODO items for a next PR? |
seiko2plus
force-pushed
the
improve_simd_doc
branch
from
ecb6090
to
18ee909
Compare
|
@rgommers, sounds good to me, for the next pr then. |
seiko2plus
changed the title
There was a problem hiding this comment.
Hello @seiko2plus ! I did a thorough read and this looks really good. I left a number of comments, most of them about grammar, typos or formatting, nothing major. One point I would like to raise is that we could move this document to the "Under the hood" section, as I think it fits better there. But I'll leave the decision to you and other maintainers. Also feel free to ignore my nitpicky comments!
doc/source/reference/simd/index.rst
Outdated
|
|
||
| The optimization process in NumPy is carried out in three layers: | ||
|
|
||
| - Code is *written* using the universal intrinsics, with guards that |
There was a problem hiding this comment.
Can we provide a link to a definition or explanation of what universal intrinsics are?
There was a problem hiding this comment.
add a simple explanation for it but I am not sure if that would be enough. we will have a separate document for it with a reference for the supported intrinsics.
| NumPy dispatcher is based on multi-source compiling, which means taking | ||
| a certain source and compiling it multiple times with different compiler | ||
| flags and also with different **C** definitions that affect the code | ||
| paths to enable certain instruction-sets for each compiled object | ||
| depending on the required optimizations, then combining the returned | ||
| objects together. |
There was a problem hiding this comment.
This is a very long sentence - might be re-worded, maybe something like:
| NumPy dispatcher is based on multi-source compiling, which means taking | |
| a certain source and compiling it multiple times with different compiler | |
| flags and also with different **C** definitions that affect the code | |
| paths to enable certain instruction-sets for each compiled object | |
| depending on the required optimizations, then combining the returned | |
| objects together. | |
| NumPy dispatcher is based on multi-source compiling, which means taking | |
| a certain source and compiling it multiple times with different compiler | |
| flags and also with different **C** definitions that affect the code | |
| paths. This enables certain instruction-sets for each compiled object | |
| depending on the required optimizations which are then combined in the returned | |
| objects together. |
There was a problem hiding this comment.
Done, but I have changed the last phrase to become "and ends with linking the
returned objects together.".
Co-authored-by: Melissa Weber Mendonça <melissawm@gmail.com>
seiko2plus
force-pushed
the
improve_simd_doc
branch
from
420ead2
to
4aade91
Compare
|
Hi @melissawm!, thank you for your amazing and accurate review. I have addressed all your points within a separate commit. Now what is left is moving this index to "Under the hood", I'm not sure, to be honest of what the right call should be but the "build options" document shouldn't be part of the "under the hood" since its targets the end-user but it makes sense to move the rest and the upcoming documents to "under the hood". I'd rather leave that decision up to you and the doc team. |
| build-options | ||
| how-it-works | ||
|
|
||
| .. _`NEP-38`: https://numpy.org/neps/nep-0038-SIMD-optimizations.html |
There was a problem hiding this comment.
This isn't needed. We have intersphinx set up for numpy.org/neps, so simply this should do it:
`NEP 38`_
The reference 6 lines up just needs the dash removed.
There was a problem hiding this comment.
This is a minor thing, so let's ignore it - maybe you can take it along in a future PR.
There was a problem hiding this comment.
I had a read through, this looks great. The features generator is a good idea, keeps the detailed listing of supported features maintainable. Thanks @seiko2plus, and thanks @melissawm for the detailed review!
related to #18926, merge after #18884
This pull request is limited to improve the following :
- [ ] CPU build options(remains runtime trace)- [ ] How to use the dispatcher API- [ ] How the dispatcher worksThis pull request will not cover the following:
Live URL https://23836-908607-gh.circle-artifacts.com/0/doc/build/html/reference/simd/index.html