Felix-Gong commented on code in PR #3601:
URL: https://github.com/apache/brpc/pull/3601#discussion_r4266518102
##########
src/butil/crc32c.cc:
##########
@@ -821,16 +821,17 @@ static inline Function Choose_Extend() {
}
#endif
#if defined(__riscv) && (__riscv_xlen == 64) && (defined(__riscv_zbc) ||
defined(__riscv_zvbc))
-#if defined(__riscv_zvbc)
- if (is_zvbc()) {
- return (Function)rv_crc32c_vclmul;
- }
-#endif
+ // Scalar Zbc first: lower setup overhead for small buffers.
#if defined(__riscv_zbc)
if (is_zbc()) {
return (Function)rv_crc32c_clmul;
}
#endif
+#if defined(__riscv_zvbc)
+ if (is_zvbc()) {
+ return (Function)rv_crc32c_vclmul;
+ }
+#endif
Review Comment:
A fixed order does drop vclmul. Switched to size-based dispatch: clmul under
256 B, vclmul from 256 B on. On X100 that keeps 11.7 GB/s at 1 MiB. Extend()
stays untouched.
##########
src/butil/crc32c.cc:
##########
@@ -821,16 +821,17 @@ static inline Function Choose_Extend() {
}
#endif
#if defined(__riscv) && (__riscv_xlen == 64) && (defined(__riscv_zbc) ||
defined(__riscv_zvbc))
-#if defined(__riscv_zvbc)
- if (is_zvbc()) {
- return (Function)rv_crc32c_vclmul;
- }
-#endif
+ // Scalar Zbc first: lower setup overhead for small buffers.
#if defined(__riscv_zbc)
if (is_zbc()) {
return (Function)rv_crc32c_clmul;
}
#endif
+#if defined(__riscv_zvbc)
+ if (is_zvbc()) {
+ return (Function)rv_crc32c_vclmul;
+ }
+#endif
Review Comment:
test-reply-probe
##########
src/butil/crc32c.cc:
##########
@@ -821,16 +821,16 @@ static inline Function Choose_Extend() {
}
#endif
#if defined(__riscv) && (__riscv_xlen == 64) && (defined(__riscv_zbc) ||
defined(__riscv_zvbc))
-#if defined(__riscv_zvbc)
- if (is_zvbc()) {
- return (Function)rv_crc32c_vclmul;
- }
-#endif
#if defined(__riscv_zbc)
if (is_zbc()) {
return (Function)rv_crc32c_clmul;
}
#endif
+#if defined(__riscv_zvbc)
+ if (is_zvbc()) {
+ return (Function)rv_crc32c_vclmul;
+ }
+#endif
Review Comment:
The RISC-V dispatch isn't exercised in CI (x86 builds), so a test needs a
hook to reach it. Verified on hardware instead: CRC.* passes and the size split
behaves as intended. Would rather skip the hook unless maintainers want it.
##########
src/butil/crc32c.cc:
##########
@@ -821,16 +821,16 @@ static inline Function Choose_Extend() {
}
#endif
#if defined(__riscv) && (__riscv_xlen == 64) && (defined(__riscv_zbc) ||
defined(__riscv_zvbc))
-#if defined(__riscv_zvbc)
- if (is_zvbc()) {
- return (Function)rv_crc32c_vclmul;
- }
-#endif
#if defined(__riscv_zbc)
if (is_zbc()) {
return (Function)rv_crc32c_clmul;
}
#endif
+#if defined(__riscv_zvbc)
+ if (is_zvbc()) {
+ return (Function)rv_crc32c_vclmul;
+ }
+#endif
Review Comment:
Added a comment on why the split is at 256 B.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
