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 |
|
|
||
| Now this introduces quite an obvious overhead of an additional load and store operation with each op. However, the compiler is able to optimize these out with the following conditions: | ||
|
|
||
| 1. The -O3 flag is set |
There was a problem hiding this comment.
-O3 can introduce other issues, including code bloat. how do you handle that?
There was a problem hiding this comment.
If I'm not mistaken, PyTorch is built with -O3 in release mode so there shouldn't be any extra code bloat
There was a problem hiding this comment.
That for pypi release, but deployment of pytorch may get built with different flags. Plus note that this being header only change also allows reuse of the class in other implementations including that of custom ops that dont live inside pytorch. And my custom op lib may not be built with O3 for size reasons. Imagine for example deploying this on mobile. so I do think we should touch upon this aspect carefully and at least call it out clearly.
| ## **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.
I think one question i have is this: true power of SVE I presume is that you can have same binary compiled for one implementation of sve work on another and realize performance gain. That is great. If this means that pytorch distribution built for arm, when pip installed, can take advantage of vector length of that machine. is that how we plan to realize performance gain?
There was a problem hiding this comment.
Yes, currently, we only have SVE support for 256bit vector machines, with this change we can do as you suggested: build one version of PyTorch for aarch64 and use it on all aarch64 machines without worrying about vector length
this doesn't seem to address my concerns about adding additional heap allocations. are there technical barriers to the plan I outlined to avoid heap allocations, such as reasons for size() to be constexpr other than determining the size of an array? We don't need benchmarking to tell us that heap allocations are worse than no heap allocations. |
So I've investigated all the places where size() needs to be constexpr here and as you suggest, a lot of it just initializing an array which can be solved by using c arrays. The other places where it's used are template parameters where we would have to move the parameter from the template to a function argument |
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