Repository navigation
cksum: only report x86 hardware features on x86 targets - #14805
Conversation
|
The fix should be made also be made in This then becomes dead code: #[cfg(not(any(target_arch = "x86", target_arch = "x86_64")))]
fn detect_avx512() -> bool {
false
} |
GNU does not implement |
|
What? GNU supports |
$ gcksum --version
cksum (GNU coreutils) 9.12
...
$ gcksum --debug /dev/null
4294967295 0 /dev/nullMy mistake, |
5b59a8f to
6dd4e91
Compare
It's not strictly a deadcode because |
Merging this PR will not alter performance
Comparing Footnotes
|
085eff8 to
7a4574c
Compare
| (HardwareFeature::Sse2, detect_sse2), | ||
| ]; | ||
|
|
||
| // The NEON implementations these features select (for example bytecount's |
There was a problem hiding this comment.
ARM Neon is fully supported in big-endian mode, would it make sense not to exclude it here?
There was a problem hiding this comment.
Thank you! Is it better now?
7a4574c to
c8675fd
Compare
| (HardwareFeature::Sse2, detect_sse2), | ||
| ]; | ||
|
|
||
| // bytecount's aarch64 SIMD code, used by wc, is only built for little-endian |
There was a problem hiding this comment.
uucore is a separately published crate, so what wc builds isn't really its concern; these features also appear to be used by cksum. I'd err on the side of portability unless little-endian is an actual requirement of the detection itself.
There was a problem hiding this comment.
So what would you suggest as a comment here?
There was a problem hiding this comment.
So removing target_endian?
|
GNU testsuite comparison: |
| (HardwareFeature::Sse2, detect_sse2), | ||
| ]; | ||
|
|
||
| // bytecount's aarch64 SIMD code, used by wc, is only built for little-endian |
There was a problem hiding this comment.
is bytecount really the reason? detect_asimd was already little-endian only before this pr
There was a problem hiding this comment.
I am not sure about the historic context of these. I'm also wondering what's the reason... The actual direct reason here is detect_asimd for little-endian has been gone below... and I am not sure if this (cleaning up detect_avx512 or detect_asimd) is a good design. See my comment above. I would personally prefer to tight the scope of this PR to cksum only.
| .succeeds() | ||
| .stdout_is_fixture("crc_single_file.expected") | ||
| .stderr_str_check(|stderr| { | ||
| stderr == "using vmull hardware support\n" |
There was a problem hiding this comment.
accepting both strings makes the check pretty thin, can't we at least assert there is a single line?
There was a problem hiding this comment.
stderr == "some string" already asserted it's a single line.
| } | ||
|
|
||
| #[test] | ||
| #[cfg(not(any(target_arch = "x86", target_arch = "x86_64", target_arch = "aarch64")))] |
There was a problem hiding this comment.
this test will never run on our CI targets, no? not sure it buys much
There was a problem hiding this comment.
riscv64 CI runs this, which was the original purpose of this PR, to fix it for riscv64.
coreutils/.github/workflows/CICD.yml
Line 377 in c8675fd
| print_feature("pclmul", features.has_pclmul()); | ||
| } | ||
|
|
||
| // ARM aarch64 |
There was a problem hiding this comment.
why not gate on little-endian here too, like uucore does?
There was a problem hiding this comment.
I am not sure if we should drop little endian gate. So is the decision to drop it in uucore or nor?
| print_feature("avx512", features.has_avx512()); | ||
| print_feature("avx2", features.has_avx2()); | ||
| print_feature("pclmul", features.has_pclmul()); | ||
| if cfg!(any(target_arch = "x86", target_arch = "x86_64")) { |
There was a problem hiding this comment.
we now list the arches here, in hardware.rs and in the tests. could uucore expose the features the target can report instead?
There was a problem hiding this comment.
Do you mean export FEATURE_DETECTORS?
There was a problem hiding this comment.
Ah I know. Working on it now
There was a problem hiding this comment.
Lots of regressions appreared when I applied this, some due to previous defects but were hidden and will be triggered by this change...
There was a problem hiding this comment.
I just restored the PR to be the original simple shape. I think we can do those hardware changes step by step in follow up PRs.
c8675fd to
c3afbfd
Compare
Signed-off-by: Zhiwei Liang <zhiwei.liang@zliang.me>
c3afbfd to
9d0fbc5
Compare
|
uucore is tooooooo complicated, and I am not an expert in this. I restored the original shape of the PR, the simple fix for |
|
I'll continue with the |
|
@ChihweiLHBird I'll defer to your PR if you want to handle that? |
No worries, feel free to keep yours |
|
@ChihweiLHBird It's just your commit right now, up to you. |
Yep, no problem, I will put it up when the PR I mentioned above is finished. Thanks! (Trying to achieve zero regression for each atomic commit/PR) |
Summary
cksum --debugprinted the avx512, avx2 and pclmul detection lines on every target, so a build for a non-x86 architecture reported instruction sets its CPU cannot have. GNU compiles each of those lines in only where the matching implementation exists, so it prints none of them on non-x86.This PR gates the three x86 lines on x86/x86_64, the same way the vmull line is already gated on aarch64, and splits the tests so the x86 assertions run only on x86 and a new test covers the other targets.
Example (assuming non-x86 arch)
$ cksum --debug /dev/null # riscv64gc-unknown-linux-musl build, run under qemu-riscv64-staticBefore:
After, same as GNU: nothing on stderr. Output on x86-64 is unchanged.