[arm64] Add SVE implementation of Array.indexOf() [v8/v8 : main]
Anton Kirilov (Gerrit)
Anton Kirilov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anton Kirilov
| Auto-Submit | +1 |
1 comment
This change doesn't cause any significant performance difference on processors with 128-bit scalable vector registers, except for microbenchmarks such as `test/js-perf-test/SixSpeed/map_set_lookup/es5.js`, where there is a 3.22% and 11.24% improvement on Cortex-A725 and Cortex-X925 respectively.
However, the situation on processors with 256-bit vectors is different. The `ai-astar` benchmark in the JetStream 2.2 suite sees a 6.07% improvement on Neoverse V1, while the value for the fore-mentioned microbenchmark is 67%.
Related details
- Darius Mercadier
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Kirilov (Gerrit)
Anton Kirilov added 1 comment![]( "Open in Gerrit")
Hello, do you have any feedback?
Related details
- Darius Mercadier
- Michael Lippautz
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 2 comments![]( "Open in Gerrit")
High-level comment: we don't have any coverage for this, right? I guess that you might have machines with SVE at Arm and you checked that it works?
Would you happen to have a list of CPUs that support SVE? Or maybe rather a list of actual devices that we can buy? Are there smartphones that support it? Is it supported by M4 CPUs by any chance?
I'm not a huge fan of landing code that we can't test. So if indeed we don't have any machines with SVE support on our testing infrastructures, I'd prefer to guard this code behind a flag (or ifdef) that is false by default (and that we'll flip to true once we are able to test it).
Hello, do you have any feedback?
Hi Anton. Sorry for the delay, I'm a bit swamped right now. I doubt I'll have time to review this week, but I'll try to review before the end of next week.
Related details
- Anton Kirilov
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Kirilov (Gerrit)
Anton Kirilov added 1 comment![]( "Open in Gerrit")
High-level comment: we don't have any coverage for this, right? I guess that you might have machines with SVE at Arm and you checked that it works?
Would you happen to have a list of CPUs that support SVE? Or maybe rather a list of actual devices that we can buy? Are there smartphones that support it? Is it supported by M4 CPUs by any chance?
I'm not a huge fan of landing code that we can't test. So if indeed we don't have any machines with SVE support on our testing infrastructures, I'd prefer to guard this code behind a flag (or ifdef) that is false by default (and that we'll flip to true once we are able to test it).
Hi, Darius! No problem, I just wanted to make sure that this change hadn't slipped through the cracks.
This is not an official endorsement, but you can find a list of SoCs and the architectural features such as SVE that they support [here](https://gpages.juszkiewicz.com.pl/arm-socs-table/arm-socs.html). AFAIK the data is crowdsourced, so mistakes are possible, but I haven't noticed any; there are certainly gaps such as data for Google Tensor G4 and G5. In terms of actual devices, all Google Pixel phones starting with 8/8a should support SVE. Furthermore, all Google cloud instances that are based on either Google Axion or NVIDIA Grace should work too. This list is not exhaustive, of course - I just assumed that Google devices would be the easiest to obtain. However, Apple products do not support SVE AFAIK.
If it is indeed infeasible to test these changes, then I would be happy to make them disabled by default.
Related details
- Darius Mercadier
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Kirilov (Gerrit)
Anton Kirilov added 1 comment![]( "Open in Gerrit")
Anton KirilovHigh-level comment: we don't have any coverage for this, right? I guess that you might have machines with SVE at Arm and you checked that it works?
Would you happen to have a list of CPUs that support SVE? Or maybe rather a list of actual devices that we can buy? Are there smartphones that support it? Is it supported by M4 CPUs by any chance?
I'm not a huge fan of landing code that we can't test. So if indeed we don't have any machines with SVE support on our testing infrastructures, I'd prefer to guard this code behind a flag (or ifdef) that is false by default (and that we'll flip to true once we are able to test it).
Hi, Darius! No problem, I just wanted to make sure that this change hadn't slipped through the cracks.
This is not an official endorsement, but you can find a list of SoCs and the architectural features such as SVE that they support [here](https://gpages.juszkiewicz.com.pl/arm-socs-table/arm-socs.html). AFAIK the data is crowdsourced, so mistakes are possible, but I haven't noticed any; there are certainly gaps such as data for Google Tensor G4 and G5. In terms of actual devices, all Google Pixel phones starting with 8/8a should support SVE. Furthermore, all Google cloud instances that are based on either Google Axion or NVIDIA Grace should work too. This list is not exhaustive, of course - I just assumed that Google devices would be the easiest to obtain. However, Apple products do not support SVE AFAIK.
If it is indeed infeasible to test these changes, then I would be happy to make them disabled by default.
Sorry, I forgot to mention that I tested the changes on 2 different platforms with different scalable vector sizes - the same machines on which I obtained the benchmark results that I stated before.
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
😿 Job android-pixel9-pro-xl-perf/jetstream3.crossbench failed.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/1561a42e090000
Related details
- Anton Kirilov
- Darius Mercadier
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
chromeperf@appspot.gserviceaccount.com (Gerrit)
Message from chrom...@appspot.gserviceaccount.com![]( "Open in Gerrit")
📍 Job android-pixel9-perf/jetstream-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15d7b295090000
Anton Kirilov (Gerrit)
Anton Kirilov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Anton Kirilov
| Auto-Submit | +1 |
Darius MercadierHello, do you have any feedback?
Hi Anton. Sorry for the delay, I'm a bit swamped right now. I doubt I'll have time to review this week, but I'll try to review before the end of next week.
@dmerc...@chromium.org Hello, did you have a chance to look at the changes?
Related details
- Darius Mercadier
- Michael Lippautz
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Darius Mercadier (Gerrit)
Darius Mercadier added 1 comment![]( "Open in Gerrit")
- Anton Kirilov
- Patrick Thier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Thier (Gerrit)
Patrick Thier voted and added 9 comments![]( "Open in Gerrit")
Votes added by Patrick Thier
| Code-Review | +1 |
9 comments
Thanks! LGTM with nits
static uint64_t vector_length(void);nit: Simply `vector_length()` in C++ code.
static __SVBool_t all_true_predicate(void);nit: Same as above. Please remove `void`
__attribute__((target("sve"))) std::optional<uint64_t> find_match_in_vector(nit: Please define a small macro for that (see `TARGET_AVX2`). Applies to all methods below, just mentioning it once here.
static_assert(base::bits::IsPowerOfTwo(sizeof(*array)));nit: `sizeof(ScalarType)` (also other occurrences of `sizeof(*array)`
DCHECK_EQ(reinterpret_cast<uintptr_t>(array) % sizeof(*array), 0);nit: `DCHECK(IsAligned(...`
reinterpret_cast<uintptr_t>(array + start_index + vector_length) &
~(vector_length * sizeof(*array) - 1);nit: We have `RoundDown` in `base/macros.h`
while (i-- != 0) {nit: IMO a `for` loop is more readable here.
return (number_of_elements & (vector_length - 1)) == 0nit: `IsAligned()`
Related details
- Anton Kirilov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Kirilov (Gerrit)
Anton Kirilov voted and added 8 comments![]( "Open in Gerrit")
Votes added by Anton Kirilov
| Auto-Submit | +1 |
8 comments
nit: Simply `vector_length()` in C++ code.
Done
nit: Same as above. Please remove `void`
Done
__attribute__((target("sve"))) std::optional<uint64_t> find_match_in_vector(nit: Please define a small macro for that (see `TARGET_AVX2`). Applies to all methods below, just mentioning it once here.
Done
static_assert(base::bits::IsPowerOfTwo(sizeof(*array)));nit: `sizeof(ScalarType)` (also other occurrences of `sizeof(*array)`
Done
DCHECK_EQ(reinterpret_cast<uintptr_t>(array) % sizeof(*array), 0);Anton Kirilovnit: `DCHECK(IsAligned(...`
Done
reinterpret_cast<uintptr_t>(array + start_index + vector_length) &
~(vector_length * sizeof(*array) - 1);nit: We have `RoundDown` in `base/macros.h`
Done
nit: IMO a `for` loop is more readable here.
Done
return (number_of_elements & (vector_length - 1)) == 0Anton Kirilovnit: `IsAligned()`
Done
Related details
- Patrick Thier
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Patrick Thier (Gerrit)
Patrick Thier voted and added 1 comment![]( "Open in Gerrit")
Votes added by Patrick Thier
| Code-Review | +1 |
- Anton Kirilov
- Igor Sheludko
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anton Kirilov (Gerrit)
Anton Kirilov added 1 comment![]( "Open in Gerrit")
Anton KirilovHigh-level comment: we don't have any coverage for this, right? I guess that you might have machines with SVE at Arm and you checked that it works?
Would you happen to have a list of CPUs that support SVE? Or maybe rather a list of actual devices that we can buy? Are there smartphones that support it? Is it supported by M4 CPUs by any chance?
I'm not a huge fan of landing code that we can't test. So if indeed we don't have any machines with SVE support on our testing infrastructures, I'd prefer to guard this code behind a flag (or ifdef) that is false by default (and that we'll flip to true once we are able to test it).
Anton KirilovHi, Darius! No problem, I just wanted to make sure that this change hadn't slipped through the cracks.
This is not an official endorsement, but you can find a list of SoCs and the architectural features such as SVE that they support [here](https://gpages.juszkiewicz.com.pl/arm-socs-table/arm-socs.html). AFAIK the data is crowdsourced, so mistakes are possible, but I haven't noticed any; there are certainly gaps such as data for Google Tensor G4 and G5. In terms of actual devices, all Google Pixel phones starting with 8/8a should support SVE. Furthermore, all Google cloud instances that are based on either Google Axion or NVIDIA Grace should work too. This list is not exhaustive, of course - I just assumed that Google devices would be the easiest to obtain. However, Apple products do not support SVE AFAIK.
If it is indeed infeasible to test these changes, then I would be happy to make them disabled by default.
Sorry, I forgot to mention that I tested the changes on 2 different platforms with different scalable vector sizes - the same machines on which I obtained the benchmark results that I stated before.
- Darius Mercadier
- Igor Sheludko
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Igor Sheludko (Gerrit)
Igor Sheludko voted and added 1 comment![]( "Open in Gerrit")
Votes added by Igor Sheludko
| Code-Review | +1 |
| Commit-Queue | +2 |
1 comment
Related details
- Anton Kirilov
- Darius Mercadier
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
v8-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
v8-s...@luci-project-accounts.iam.gserviceaccount.com submitted the change![]( "Open in Gerrit")
Change information
[arm64] Add SVE implementation of Array.indexOf()
The implementation is vector length-agnostic (VLA), i.e. it works
correctly for any scalable vector register size permitted by the 64-bit
Arm architecture.
- M src/base/cpu/cpu-arm.cc
- M src/base/cpu/cpu.h
- M src/base/macros.h
- M src/codegen/arm64/assembler-arm64.cc
- M src/codegen/cpu-features.h
- M src/objects/simd.cc
Code-Review: +1 by Igor Sheludko, +1 by Patrick Thier
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Deepti Gandluri (Gerrit)
Deepti Gandluri has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes added 1 comment![]( "Open in Gerrit")
This also failed on the roll: https://crrev.com/c/7763830
```
../../v8/src/objects/simd.cc:57:32: error: use of undeclared identifier 'SVE'
57 | if (CpuFeatures::IsSupported(SVE)) {
| ^~~
```
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Achenbach (Gerrit)
Michael Achenbach added 1 comment![]( "Open in Gerrit")
Looks like this also blocked the roll:
https://crrev.com/c/7762881
https://ci.chromium.org/ui/p/chromium/builders/try/mac_optional_gpu_tests_rel/190362/overview
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |