Felix-Gong commented on code in PR #3601:
URL: https://github.com/apache/brpc/pull/3601#discussion_r4267326910
##########
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:
Extracted it to `kRiscvVclmulThresholdBytes`, next to `RiscvExtend`. It only
exists in the Zbc+Zvbc build, and the comment above still explains why the
crossover is around there.
##########
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` has the same signature as `Function`, so the cast isn't needed
for correctness. I kept it because every other return in this function casts
explicitly (`(Function)rv_crc32c_clmul`, `(Function)ExtendImpl<...>`), and
matching that reads better than one uncast return in the middle.
##########
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:
Changed to `#elif defined(__riscv_zbc)`. The outer condition already
requires zbc or zvbc, so behavior is unchanged, but writing it out makes the
intent clear if that condition is ever edited.
##########
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:
Same as above — made the branch explicit.
--
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]
