Copilot commented on code in PR #3601:
URL: https://github.com/apache/brpc/pull/3601#discussion_r4266278185
##########
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:
With this ordering, CPUs that have both Zbc and Zvbc will never use
`rv_crc32c_vclmul`, even for large buffers where the vector path may become
faster as the implementation evolves (or on certain microarchitectures). If the
intent is only to avoid *small-buffer* regressions while still allowing
large-buffer wins, consider returning a small wrapper dispatcher when both are
supported that selects clmul below a tuned size threshold and vclmul above it
(threshold ideally backed by your bench numbers). Also consider updating the
comment to clarify that this is a strict preference (not just a small-buffer
heuristic) if you intend to always force scalar when Zbc is present.
--
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]
