Skip to content

Conversation

@tarcieri
Copy link
Member

@tarcieri tarcieri commented Dec 28, 2025

The implementation was using the wrong register type when invoking csel for u64 (:w which is 32-bit, should use :x for 64-bit)

This captures the bug in a regression test and fixes it.

@tarcieri
Copy link
Member Author

The PPC failure is probably the same issue as #1298.

GitHub Actions killed the aarch64 runs but they're also failing:

https://github.com/RustCrypto/utils/actions/runs/20548849075/job/59023609534?pr=1299

Run cross test --target aarch64-unknown-linux-gnu
[...]
     Running tests/regression.rs (/target/aarch64-unknown-linux-gnu/debug/deps/regression-d4120ddf24ad0571)

running 1 test
test u64_cmoveq ... FAILED

failures:

---- u64_cmoveq stdout ----

thread 'u64_cmoveq' panicked at cmov/tests/regression.rs:12:5:
assertion `left == right` failed
  left: 1
 right: 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I sprinkled some dbg! into the u64 impls in aarch64.rs like so:

#[inline]
fn cmoveq(&self, rhs: &Self, input: Condition, output: &mut Condition) {
    dbg!(self, rhs, input, &output);
    csel_eq!("csel {3:w}, {4:w}, {5:w}, EQ", self, rhs, input, output);
}

It's definitely getting called, here's the output:

---- u64_cmoveq stdout ----
[cmov/src/aarch64.rs:110:9] self = 9367487224930631680
[cmov/src/aarch64.rs:110:9] rhs = 0
[cmov/src/aarch64.rs:110:9] input = 1
[cmov/src/aarch64.rs:110:9] &output = 0

thread 'u64_cmoveq' panicked at cmov/tests/regression.rs:12:5:
assertion `left == right` failed
  left: 1
 right: 0
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

cc @brxken128 @codahale who worked on the aarch64 impl

@tarcieri
Copy link
Member Author

tarcieri commented Dec 28, 2025

It seems the registers should be :x instead of :w but that doesn't seem to actually fix the problem

Edit: aah, looks like the other macros hardcode :w too and that might need to be changed to :x?

Edit again: confirm changing all of the asm registers from :w to :x fixes the problem

@tarcieri tarcieri force-pushed the cmov/u64-cmoveq-bug branch 2 times, most recently from 9eb8a56 to fc58772 Compare December 28, 2025 05:11
The implementation was using the wrong register type when invoking
`csel` for `u64` (`:w` which is 32-bit, should use `:x` for 64-bit)

This captures the bug in a regression test and fixes it.
@tarcieri tarcieri force-pushed the cmov/u64-cmoveq-bug branch from fc58772 to f17686b Compare December 28, 2025 05:12
@tarcieri tarcieri changed the title cmov: add regression test for u64 bug cmov: fix aarch64 ASM bug; add regression test Dec 28, 2025
@tarcieri tarcieri merged commit 27b9819 into master Dec 28, 2025
20 checks passed
@tarcieri tarcieri deleted the cmov/u64-cmoveq-bug branch December 28, 2025 05:14
@tarcieri tarcieri mentioned this pull request Dec 28, 2025
tarcieri added a commit that referenced this pull request Dec 28, 2025
### Fixed
- `aarch64` ASM bug (#1299)
- Truncation bug in portable implementation (#1300)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants