[riscv] Enable highway for RISC-V [v8/v8 : main]
Matthias Liedtke (Gerrit)
Matthias Liedtke voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matthias Liedtke
| Code-Review | +1 |
1 comment
Related details
- Florian Loitsch
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jakob Kummerow (Gerrit)
Jakob Kummerow voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jakob Kummerow
| Code-Review | +1 |
2 comments
LGTM with a comment.
static HWY_LANES_CONSTEXPR const size_t stride = hw::Lanes(tag);From another CL today that tried to do this, I believe this is what's currently making the Node bot red. You may want to skip it for now.
Related details
- Florian Loitsch
- 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. |
Michael Lippautz (Gerrit)
Michael Lippautz added 1 comment![]( "Open in Gerrit")
I have seen some issues today with the roll of highway.
+bikineev as I think he is better aware of whether the roll will stick
Related details
- Anton Bikineev
- Florian Loitsch
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 added 1 comment![]( "Open in Gerrit")
I would suggest removing the changes in json-stringifier.cc and runtime-regexp.cc in this CL. They don't seem necessary to enable highway for RISC-V and we need to update the library in more places manually.
Related details
- Anton Bikineev
- Florian Loitsch
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Loitsch (Gerrit)
Florian Loitsch added 2 comments![]( "Open in Gerrit")
I would suggest removing the changes in json-stringifier.cc and runtime-regexp.cc in this CL. They don't seem necessary to enable highway for RISC-V and we need to update the library in more places manually.
When compiling with vector support, they are necessary:
```
../../src/json/json-stringifier.cc:3392:33: error: invalid operands to binary expression ('const VFromD<Simd<unsigned char, 16, 0>>' (aka 'const __rvv_uint8m1_t') and 'const vuint8m1_t' (aka 'const __rvv_uint8m1_t'))
3392 | const auto has_0x22 = input == mask_0x22;
| ~~~~~ ^ ~~~~~~~~~
../../src/json/json-stringifier.cc:3393:33: error: invalid operands to binary expression ('const VFromD<Simd<unsigned char, 16, 0>>' (aka 'const __rvv_uint8m1_t') and 'const vuint8m1_t' (aka 'const __rvv_uint8m1_t'))
3393 | const auto has_0x5c = input == mask_0x5c;
| ~~~~~ ^ ~~~~~~~~~
```
To get these errors, the riscv cross-compilation target must be changed to one that has vector support. I plan to change the default target in a future CL.
What I could do: fix the errors independently of enabling highway for riscv, and split the CL in two. I'm mostly worried that reverting it (for example because the highway CL is reverted) would then become more annoying.
LGTM but not an owner. 😊
- Anton Bikineev
- Matthias Liedtke
- 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 added 1 comment![]( "Open in Gerrit")
Florian LoitschI would suggest removing the changes in json-stringifier.cc and runtime-regexp.cc in this CL. They don't seem necessary to enable highway for RISC-V and we need to update the library in more places manually.
When compiling with vector support, they are necessary:
```
../../src/json/json-stringifier.cc:3392:33: error: invalid operands to binary expression ('const VFromD<Simd<unsigned char, 16, 0>>' (aka 'const __rvv_uint8m1_t') and 'const vuint8m1_t' (aka 'const __rvv_uint8m1_t'))
3392 | const auto has_0x22 = input == mask_0x22;
| ~~~~~ ^ ~~~~~~~~~
../../src/json/json-stringifier.cc:3393:33: error: invalid operands to binary expression ('const VFromD<Simd<unsigned char, 16, 0>>' (aka 'const __rvv_uint8m1_t') and 'const vuint8m1_t' (aka 'const __rvv_uint8m1_t'))
3393 | const auto has_0x5c = input == mask_0x5c;
| ~~~~~ ^ ~~~~~~~~~
```To get these errors, the riscv cross-compilation target must be changed to one that has vector support. I plan to change the default target in a future CL.
What I could do: fix the errors independently of enabling highway for riscv, and split the CL in two. I'm mostly worried that reverting it (for example because the highway CL is reverted) would then become more annoying.
Oh I just realized that operator overloads are currently unsupported on RVV.
So this change is fine, but please add a TODO to change the comparisons back to operators once they are supported for RVV.
My original comment was focused more on the introduction of `HWY_LANES_CONSTEXPR`. We can't use that until we have the library updated to 1.3.0 everywhere. So please just remove this change from this CL, then this should work on all bots.
Related details
- Anton Bikineev
- Florian Loitsch
- Matthias Liedtke
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Loitsch (Gerrit)
Florian Loitsch added 2 comments![]( "Open in Gerrit")
Florian LoitschI would suggest removing the changes in json-stringifier.cc and runtime-regexp.cc in this CL. They don't seem necessary to enable highway for RISC-V and we need to update the library in more places manually.
Patrick ThierWhen compiling with vector support, they are necessary:
```
../../src/json/json-stringifier.cc:3392:33: error: invalid operands to binary expression ('const VFromD<Simd<unsigned char, 16, 0>>' (aka 'const __rvv_uint8m1_t') and 'const vuint8m1_t' (aka 'const __rvv_uint8m1_t'))
3392 | const auto has_0x22 = input == mask_0x22;
| ~~~~~ ^ ~~~~~~~~~
../../src/json/json-stringifier.cc:3393:33: error: invalid operands to binary expression ('const VFromD<Simd<unsigned char, 16, 0>>' (aka 'const __rvv_uint8m1_t') and 'const vuint8m1_t' (aka 'const __rvv_uint8m1_t'))
3393 | const auto has_0x5c = input == mask_0x5c;
| ~~~~~ ^ ~~~~~~~~~
```To get these errors, the riscv cross-compilation target must be changed to one that has vector support. I plan to change the default target in a future CL.
What I could do: fix the errors independently of enabling highway for riscv, and split the CL in two. I'm mostly worried that reverting it (for example because the highway CL is reverted) would then become more annoying.
Oh I just realized that operator overloads are currently unsupported on RVV.
So this change is fine, but please add a TODO to change the comparisons back to operators once they are supported for RVV.My original comment was focused more on the introduction of `HWY_LANES_CONSTEXPR`. We can't use that until we have the library updated to 1.3.0 everywhere. So please just remove this change from this CL, then this should work on all bots.
Done
static HWY_LANES_CONSTEXPR const size_t stride = hw::Lanes(tag);From another CL today that tried to do this, I believe this is what's currently making the Node bot red. You may want to skip it for now.
- Anton Bikineev
- Matthias Liedtke
- Patrick Thier
Code-Owners
Code-Review
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 |
1 comment
Related details
- Anton Bikineev
- Florian Loitsch
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florian Loitsch (Gerrit)
Florian Loitsch added 1 comment![]( "Open in Gerrit")
@biki...@chromium.org I think the non-BUILD.gn changes have been reviewed and feedback has been addressed.
Related details
- Anton Bikineev
- Matthias Liedtke
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |