Copilot commented on code in PR #3601:
URL: https://github.com/apache/brpc/pull/3601#discussion_r4258241434
##########
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:
This changes runtime behavior (which implementation is selected) when both
Zbc and Zvbc are available. Consider adding a targeted unit test that locks in
the intended precedence (Zbc preferred over Zvbc), e.g., by refactoring
capability detection behind an injectable hook or a test override so the
selection can be validated without depending on `/proc/cpuinfo`.
##########
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 dispatch ordering is performance-sensitive and was changed to address a
regression when both Zbc and Zvbc are present. Consider adding a short comment
here documenting why scalar Zbc must be checked first (e.g., vector setup/merge
overhead and prior regression reference), to reduce the risk of future
reordering reintroducing the issue.
--
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]
