Repository navigation
uucore: name hardware features after the CPU instructions - #15232
ChihweiLHBird wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The rename is complete and preserves existing detection and output behavior.
0 open findings
What changed in this PR
Renames hardware feature APIs to match the detected CPU instructions without changing user-visible debug labels or behavior.
Changes:
- Renames PCLMULQDQ and PMULL feature variants, methods, and detectors.
- Updates
cksumandwccall sites while preserving output labels.
| File | Description |
|---|---|
src/uucore/src/lib/features/hardware.rs |
Renames hardware feature APIs. |
src/uu/cksum/src/cksum.rs |
Uses the renamed detection methods. |
src/uu/wc/src/wc.rs |
Updates variants while retaining existing labels. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Merging this PR will degrade performance by 31.01%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | factor_three_39_bit_primes |
321.7 ms | 1,267.3 ms | -74.61% |
| ❌ | Simulation | factor_five_38_bit_primes |
1.5 s | 1.9 s | -16.92% |
| ⚡ |
Simulation | du_all_wide_tree[(5000, 500)] |
35.7 ms | 34.3 ms | +4.17% |
| ⚡ | Memory | du_all_wide_tree[(5000, 500)] |
1,041.8 KB | 1,010.5 KB | +3.1% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ChihweiLHBird:zhiwei/hardware-feature-names (556b621) with main (1747ba1)
Footnotes
-
70 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
b9051d8 to
53687b2
Compare
| "AVX2" => Ok(Avx2), | ||
| "PCLMUL" | "PMULL" => Ok(PclMul), | ||
| "VMULL" => Ok(Vmull), | ||
| "PCLMUL" | "PMULL" => Ok(Pclmulqdq), |
There was a problem hiding this comment.
with the new names, "PMULL" => Pclmulqdq looks wrong, is that intended?
There was a problem hiding this comment.
Yes, it's intended to keep this as a non-behavior-change PR and avoid a large hard-to-review diff. But I do have a plan to change them later after fixing another bug it's relying on for preventing regressions.
A sample of such regression of we make the change now for the keys here:
With GLIBC_TUNABLES=glibc.cpu.hwcaps=-PMULL
GNU: cksum: pclmul support not detected
main: cksum: pclmul support not detected
keys changed: cksum: using pclmul hardware support
`PclMul` and `Vmull` were named after the cksum CRC paths, not after the PCLMULQDQ and PMULL instructions that they detect. Rename them, and their `has_*` and `detect_*` functions, after the instructions. Signed-off-by: Zhiwei Liang <zhiwei.liang@zliang.me>
53687b2 to
556b621
Compare
Summary
HardwareFeature::PclMulandHardwareFeature::Vmulltake their names from the two CRC paths thatcksum --debugreports aspclmulandvmull. But they detect CPU instructions:is_x86_feature_detected!("pclmulqdq")andis_aarch64_feature_detected!("pmull"). The distinction matters because a path needs more than its instruction. For example, thepclmulpath also needs AVX, so a check such ashas_pclmul() && has_avx()(in a follow up PR) reads as if pclmul needs more than pclmul.This PR renames the variants to
PclmulqdqandPmull, withhas_pclmulqdq(),has_pmull()and thedetect_*functions to match. Now the enum names the instruction andcksumnames the path. Behavior does not change:cksum --debugstill printspclmulandvmull, and theGLIBC_TUNABLESnames andwc --debuglabels stay the same but may be updated in the follow up PRs to align with GNU behaviors.No behavior change is intended in this PR.