Copilot commented on code in PR #3601:
URL: https://github.com/apache/brpc/pull/3601#discussion_r4267140085
##########
src/butil/crc32c.cc:
##########
@@ -814,19 +814,40 @@ static bool isSSE42() {
typedef uint32_t (*Function)(uint32_t, const char*, size_t);
+#if defined(__riscv) && (__riscv_xlen == 64) && defined(__riscv_zbc) &&
defined(__riscv_zvbc)
+// Both Zbc and Zvbc: scalar is cheaper for small inputs, the vector fold
+// wins from ~256 B on.
+static uint32_t RiscvExtend(uint32_t crc, const char* buf, size_t size) {
+ return (size >= 256 ? rv_crc32c_vclmul : rv_crc32c_clmul)(crc, buf, size);
Review Comment:
The `256` threshold is a newly introduced magic number. Please extract it
into a named constant (e.g., `kRiscvVclmulThresholdBytes`) near the function or
in shared CRC32C dispatch constants, so the rationale is centralized and future
tuning doesn’t require hunting down literals.
##########
src/butil/crc32c.cc:
##########
@@ -814,19 +814,40 @@ static bool isSSE42() {
typedef uint32_t (*Function)(uint32_t, const char*, size_t);
+#if defined(__riscv) && (__riscv_xlen == 64) && defined(__riscv_zbc) &&
defined(__riscv_zvbc)
+// Both Zbc and Zvbc: scalar is cheaper for small inputs, the vector fold
+// wins from ~256 B on.
+static uint32_t RiscvExtend(uint32_t crc, const char* buf, size_t size) {
+ return (size >= 256 ? rv_crc32c_vclmul : rv_crc32c_clmul)(crc, buf, size);
+}
+#endif
+
static inline Function Choose_Extend() {
#ifdef __SSE4_2__
if (isSSE42()) {
return (Function)ExtendImpl<FastCRC32Functor>;
}
#endif
#if defined(__riscv) && (__riscv_xlen == 64) && (defined(__riscv_zbc) ||
defined(__riscv_zvbc))
-#if defined(__riscv_zvbc)
+#if defined(__riscv_zbc) && defined(__riscv_zvbc)
+ {
+ static const bool has_zbc = is_zbc();
+ static const bool has_zvbc = is_zvbc();
+ if (has_zbc && has_zvbc) {
+ return (Function)RiscvExtend;
Review Comment:
`RiscvExtend` already matches the `Function` typedef signature, so the
C-style cast is redundant and can hide signature mismatches if the typedef
changes. Prefer returning it without a cast (or explicitly as `&RiscvExtend`)
for safer dispatch.
##########
src/butil/crc32c.cc:
##########
@@ -814,19 +814,40 @@ static bool isSSE42() {
typedef uint32_t (*Function)(uint32_t, const char*, size_t);
+#if defined(__riscv) && (__riscv_xlen == 64) && defined(__riscv_zbc) &&
defined(__riscv_zvbc)
+// Both Zbc and Zvbc: scalar is cheaper for small inputs, the vector fold
+// wins from ~256 B on.
+static uint32_t RiscvExtend(uint32_t crc, const char* buf, size_t size) {
+ return (size >= 256 ? rv_crc32c_vclmul : rv_crc32c_clmul)(crc, buf, size);
+}
+#endif
+
static inline Function Choose_Extend() {
#ifdef __SSE4_2__
if (isSSE42()) {
return (Function)ExtendImpl<FastCRC32Functor>;
}
#endif
#if defined(__riscv) && (__riscv_xlen == 64) && (defined(__riscv_zbc) ||
defined(__riscv_zvbc))
-#if defined(__riscv_zvbc)
+#if defined(__riscv_zbc) && defined(__riscv_zvbc)
+ {
+ static const bool has_zbc = is_zbc();
+ static const bool has_zvbc = is_zvbc();
+ if (has_zbc && has_zvbc) {
+ return (Function)RiscvExtend;
+ }
+ if (has_zvbc) {
+ return (Function)rv_crc32c_vclmul;
+ }
+ if (has_zbc) {
+ return (Function)rv_crc32c_clmul;
+ }
+ }
+#elif defined(__riscv_zvbc)
if (is_zvbc()) {
return (Function)rv_crc32c_vclmul;
}
-#endif
-#if defined(__riscv_zbc)
+#else
Review Comment:
This `#else` branch implicitly means 'only `__riscv_zbc` is defined' due to
the outer condition, but that coupling is non-obvious. Consider making it
explicit as `#elif defined(__riscv_zbc)` to document intent and reduce the risk
of accidental mis-compilation if the outer preprocessor condition is modified
later.
##########
src/butil/crc32c.cc:
##########
@@ -814,19 +814,40 @@ static bool isSSE42() {
typedef uint32_t (*Function)(uint32_t, const char*, size_t);
+#if defined(__riscv) && (__riscv_xlen == 64) && defined(__riscv_zbc) &&
defined(__riscv_zvbc)
+// Both Zbc and Zvbc: scalar is cheaper for small inputs, the vector fold
+// wins from ~256 B on.
+static uint32_t RiscvExtend(uint32_t crc, const char* buf, size_t size) {
+ return (size >= 256 ? rv_crc32c_vclmul : rv_crc32c_clmul)(crc, buf, size);
+}
+#endif
+
static inline Function Choose_Extend() {
#ifdef __SSE4_2__
if (isSSE42()) {
return (Function)ExtendImpl<FastCRC32Functor>;
}
#endif
#if defined(__riscv) && (__riscv_xlen == 64) && (defined(__riscv_zbc) ||
defined(__riscv_zvbc))
-#if defined(__riscv_zvbc)
+#if defined(__riscv_zbc) && defined(__riscv_zvbc)
+ {
+ static const bool has_zbc = is_zbc();
+ static const bool has_zvbc = is_zvbc();
+ if (has_zbc && has_zvbc) {
+ return (Function)RiscvExtend;
+ }
+ if (has_zvbc) {
+ return (Function)rv_crc32c_vclmul;
+ }
+ if (has_zbc) {
+ return (Function)rv_crc32c_clmul;
+ }
+ }
+#elif defined(__riscv_zvbc)
Review Comment:
This `#else` branch implicitly means 'only `__riscv_zbc` is defined' due to
the outer condition, but that coupling is non-obvious. Consider making it
explicit as `#elif defined(__riscv_zbc)` to document intent and reduce the risk
of accidental mis-compilation if the outer preprocessor condition is modified
later.
--
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]
