RFC for vector length agnostic SVE Vectorized class #73
Conversation
|
Hi @Ryo-not-rio! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
RFC-0044-sve-vectorized-class.md
Outdated
| There are a number of functions that use the size() function to initialize an array. These will have to be changed to an alternative such as a vector. Since a vector is implemented as an array under the hood, we hope this will not cause any regressions but a thorough benchmarking of these functions need to be done to ensure that this is the case. | ||
|
|
||
| ## **Alternatives** | ||
| To keep the size() function constexpr, we considered setting the size of the Vectorized class to be the maximum possible SVE vector length (currently 512 bits) and loading multiple vectors as necessary. However, this poses the following problems: |
There was a problem hiding this comment.
Might be worth clarifying that 2048 is the maximum possible, 512 is the maximum available hardware
There was a problem hiding this comment.
Is "size() determines the size of an array" the only use of constexpr size()? If so, we should have a few options to solve that problem:
- rely on the VLA extension (I'm mobile so I can't look up which compilers do and don't support this, but I'd expect clang and gcc to get it right and msvc to be annoying) to size arrays this way anyway; we would just have to use C arrays instead of std::array
- use some template metaprogramming magic to create a container templated on vectorized that detects whether size is constexpr. Create an array if so, and either a padded-to-max-length-for-sve array or a vector otherwise.
There was a problem hiding this comment.
Some quick research says we can generally detect whether a function is constexpr when we get C++20, which should be soon: https://quuxplusone.github.io/blog/2022/01/04/test-constexpr-friendliness/
However, since this is a very specific case, we could just create a custom vectorized_of_t_has_constexpr_size traits class that defaults to true and is specialized for VLA-SVE-specialized Vectorized types to say false.
There was a problem hiding this comment.
Another option to keep size() constexpr is to use a hybrid approach where we decide the size() on compile time. This will keep size() constexpr but will not be able to take advantage of SVE's runtime vector length detection feature
RFC-0044-sve-vectorized-class.md
Outdated
| Ensuring these conditions are met and by inlining the functions, we can rely on the compiler to optimize the duplicate load and stores, ensuring we do not introduce any regressions. | ||
|
|
||
| ### The size problem | ||
| We face a challenge with this implementation due to the constraint of the size() function being constexpr. The size() function which returns the number of elements in the Vectorized class cannot be constexpr in our implmentation due to SVE vector lengths being unknown at compile time. We propose we change this to be const instead of constexpr. |
There was a problem hiding this comment.
It may be worth noting that I think a const virtual function can be overridden by a constexpr function. And even if not, I suspect in many cases the compiler should be able to work it out if the class is concrete
There was a problem hiding this comment.
I don't think there are virtual functions involved; architectures specialize Vectorized
|
I left a few minor comments, but I think overall it looks promising! Thank you |
There was a problem hiding this comment.
cc @malfet @swolchok @kimishpatel @guangy10 as people working on this topic in case you have any comments
thanks Alban. Will take a look tomorrow or friday |
RFC-0044-sve-vectorized-class.md
Outdated
| This is a large change which requires an overhaul of all of the current SVE Vectorized as well as any code that expects the size() function to be constexpr. The first cost can be mitigated by updating the Vectorized classes one by one, but the size() change will need to be done all at once. | ||
|
|
||
| ### Sideffects from non-constexpr size() | ||
| There are a number of functions that use the size() function to initialize an array. These will have to be changed to an alternative such as a vector. Since a vector is implemented as an array under the hood, we hope this will not cause any regressions but a thorough benchmarking of these functions need to be done to ensure that this is the case. |
There was a problem hiding this comment.
I would be concerned about this. Arrays go on the stack; vectors go on the heap.
RFC-0044-sve-vectorized-class.md
Outdated
| There are a number of functions that use the size() function to initialize an array. These will have to be changed to an alternative such as a vector. Since a vector is implemented as an array under the hood, we hope this will not cause any regressions but a thorough benchmarking of these functions need to be done to ensure that this is the case. | ||
|
|
||
| ## **Alternatives** | ||
| To keep the size() function constexpr, we considered setting the size of the Vectorized class to be the maximum possible SVE vector length (currently 512 bits) and loading multiple vectors as necessary. However, this poses the following problems: |
There was a problem hiding this comment.
Is "size() determines the size of an array" the only use of constexpr size()? If so, we should have a few options to solve that problem:
- rely on the VLA extension (I'm mobile so I can't look up which compilers do and don't support this, but I'd expect clang and gcc to get it right and msvc to be annoying) to size arrays this way anyway; we would just have to use C arrays instead of std::array
- use some template metaprogramming magic to create a container templated on vectorized that detects whether size is constexpr. Create an array if so, and either a padded-to-max-length-for-sve array or a vector otherwise.
| 2. The `svptrue_b32()` predicate is used | ||
| 3. You are storing to and then loading from the same pointer | ||
|
|
||
| Ensuring these conditions are met and by inlining the functions, we can rely on the compiler to optimize the duplicate load and stores, ensuring we do not introduce any regressions. |
There was a problem hiding this comment.
Is there any risk of different behaviour across different compilers/versions?
There was a problem hiding this comment.
Yes, I guess we'll have to make sure this optimization happens on all our supported compilers
| ## **Metrics ** | ||
| - Reduction of code duplication | ||
| - Speedup of PyTorch on SVE machines with non-256 bit vectors | ||
| - Softmax sped up by 2.73x on Neoverse V2 |
There was a problem hiding this comment.
Are these numbers similar for eager mode or compile?
|
|
||
| Due to these issues combined, especially 2., this alternative introduces a ~30% overhead compared to the current implementation. | ||
|
|
||
| ## **Unresolved questions** |
There was a problem hiding this comment.
Are there any plans to add specific tests?
There was a problem hiding this comment.
For unit tests, the existing tests should cover us, for perf tests, we should run the whole set of integration tests
|
@swolchok I've scoped out the all the changes that need to be done to change |
This is a proposal for a Vectorized class for SVE to replace the current Vectorized class which will make it vector length agnostic, avoiding code duplication in the future