CRC32C dispatch: prefer scalar Zbc over Zvbc (fix PR3332 small-buffer regression) by Felix-Gong · Pull Request #3601 · apache/brpc

Felix-Gong · 2026-10-13T09:23:01Z

Follow-up to #3571.

PR3332 sent every Zbc+Zvbc CPU to the vclmul path. Now that #3571
rewrote it, vclmul wins above ~256 B (1.5x-7.5x) but loses below it,
so a fixed order either way is wrong.

Pick by size: clmul under 256 B, vclmul from 256 B on. CPUs with only
Zbc or only Zvbc keep their previous path; Extend() is unchanged.

size	clmul	vclmul
256 B	1534	2155
4 KiB	1578	9411
1 MiB	1555	11702

Tested on k3 (X100): CRC.* 5/5 PASSED, 180 size/boundary combos match
the table reference.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

2 open findings

This changes runtime behavior (which implementation is selected) when both Zbc and Zvbc are… · New
The dispatch ordering is performance-sensitive and was changed to address a regression when both… · New

What changed in this PR

Adjusts the RISC-V CRC32C implementation dispatch to avoid a small-buffer performance regression by preferring the faster scalar Zbc path over the Zvbc vector path when both are available.

Changes:

Reordered Choose_Extend() dispatch checks to prefer Zbc (scalar) before Zvbc (vector).
Kept Zvbc as a fallback when Zbc isn’t available.

File	Description
`src/butil/crc32c.cc`	Reorders RISC-V extension detection to select scalar Zbc before vector Zvbc to avoid perf regressions.

🧠 Review effort: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

3 open findings

With this ordering, CPUs that have both Zbc and Zvbc will never use rv_crc32c_vclmul, even for… · New
This changes runtime behavior (which implementation is selected) when both Zbc and Zvbc are…
The dispatch ordering is performance-sensitive and was changed to address a regression when both…

🧠 Review effort: Lite

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Copilot review overview

7 open findings

The 256 threshold is a newly introduced magic number. Please extract it into a named constant… · New
With this ordering, CPUs that have both Zbc and Zvbc will never use rv_crc32c_vclmul, even for…
This changes runtime behavior (which implementation is selected) when both Zbc and Zvbc are…
RiscvExtend already matches the Function typedef signature, so the C-style cast is redundant… · New
This #else branch implicitly means 'only __riscv_zbc is defined' due to the outer condition,… · New
This #else branch implicitly means 'only __riscv_zbc is defined' due to the outer condition,… · New
The dispatch ordering is performance-sensitive and was changed to address a regression when both…

🧠 Review effort: Lite

Felix-Gong · 2026-10-14T06:04:07Z

+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);


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.

Felix-Gong · 2026-10-14T06:03:47Z

+    static const bool has_zbc = is_zbc();
+    static const bool has_zvbc = is_zvbc();
+    if (has_zbc && has_zvbc) {
+      return (Function)RiscvExtend;


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.

Felix-Gong · 2026-10-14T06:03:49Z

+      return (Function)rv_crc32c_clmul;
+    }
+  }
+#elif defined(__riscv_zvbc)


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.

Felix-Gong · 2026-10-14T06:04:09Z

  }
-#endif
-#if defined(__riscv_zbc)
+#else


Same as above — made the branch explicit.

PR3332 sent every Zbc+Zvbc CPU to the vclmul path. After apache#3571 rewrote it, vclmul beats clmul from ~256 B up (1.5x-7.5x) but costs more below that, where vector setup dominates. A fixed order to either path is wrong for these CPUs. Select by size in the dispatcher: clmul under 256 B, vclmul from 256 B on. CPUs with only Zbc or only Zvbc keep their current path, and Extend() is untouched. k3 (X100, VLEN=256, 20-round median): 256 B clmul 1534 vclmul 2155 1 MiB clmul 1555 vclmul 11702 (MB/s) Signed-off-by: Xiaofei Gong <gongxiaofei24@iscas.ac.cn> Signed-off-by: YuanSheng <yuansheng@isrc.iscas.ac.cn>

Felix-Gong force-pushed the riscv-crc32c-zvbc-dispatch branch from aaaec4c to 1b3eb93 Compare October 13, 2026 09:26

chenBright requested a lite review from Copilot October 13, 2026 11:33

Copilot AI reviewed Oct 13, 2026

View reviewed changes

Comment thread src/butil/crc32c.cc Outdated

Comment thread src/butil/crc32c.cc Outdated

Copilot stopped reviewing on behalf of chenBright due to an error October 13, 2026 16:55
The job was not acquired by a runner even after multiple attempts.

Felix-Gong force-pushed the riscv-crc32c-zvbc-dispatch branch from 1b3eb93 to 6d8ce6e Compare October 14, 2026 00:34

wwbmmm requested a lite review from Copilot October 14, 2026 02:36

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Comment thread src/butil/crc32c.cc Outdated

Felix-Gong force-pushed the riscv-crc32c-zvbc-dispatch branch from 6d8ce6e to d099479 Compare October 14, 2026 03:42

Copilot stopped reviewing on behalf of wwbmmm due to an error October 14, 2026 04:00
Workflow completed but conclusion is not available. This may indicate rate limiting or API issues.

Felix-Gong force-pushed the riscv-crc32c-zvbc-dispatch branch from d099479 to 7e618c1 Compare October 14, 2026 05:15

wwbmmm requested a lite review from Copilot October 14, 2026 05:16

Copilot AI reviewed Oct 14, 2026

View reviewed changes

Copilot stopped reviewing on behalf of wwbmmm due to an error October 14, 2026 05:38
The job was not acquired by a runner even after multiple attempts.

Felix-Gong force-pushed the riscv-crc32c-zvbc-dispatch branch from 7e618c1 to 0bee1b6 Compare October 14, 2026 06:06

Navigation Menu

Repository navigation

CRC32C dispatch: prefer scalar Zbc over Zvbc (fix PR3332 small-buffer regression) - #3601

CRC32C dispatch: prefer scalar Zbc over Zvbc (fix PR3332 small-buffer regression)#3601
Felix-Gong wants to merge 1 commit into
apache:masterfrom
Felix-Gong:riscv-crc32c-zvbc-dispatch

Felix-Gong commented Oct 13, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Felix-Gong Oct 14, 2026 •

edited

Loading

Uh oh!

Felix-Gong Oct 14, 2026 •

edited

Loading

Uh oh!

Felix-Gong Oct 14, 2026 •

edited

Loading

Uh oh!

Felix-Gong Oct 14, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		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);

Conversation

Felix-Gong commented Oct 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

Uh oh!

Felix-Gong Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Felix-Gong Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Felix-Gong Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Felix-Gong Oct 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Felix-Gong commented Oct 13, 2026 •

edited

Loading

Felix-Gong Oct 14, 2026 •

edited

Loading

Felix-Gong Oct 14, 2026 •

edited

Loading

Felix-Gong Oct 14, 2026 •

edited

Loading

Felix-Gong Oct 14, 2026 •

edited

Loading