cranelift: aarch64: zero-extend expected value in i32 atomic_cas loop - #14525
Open
kevaundray wants to merge 2 commits into
Open
kevaundray wants to merge 2 commits into
kevaundray wants to merge 2 commits into
Conversation
On aarch64 without LSE, `atomic_cas.i32` is lowered to an `AtomicCASLoop` whose comparison is `cmp x27, x26`: a 64-bit compare of the zero-extended loaded value against the whole 64-bit register holding the expected value. The upper 32 bits of a register holding an `i32` are unspecified in the aarch64 backend (e.g. `ireduce.i32` of an `i64` reuses the same register), so when they are nonzero the compare fails even though the low 32 bits match: the exchange is not performed and the old value is returned. Add a runtest exercising this (`ireduce.i32` of an `i64` with nonzero upper bits as the expected value) and a precise-output test showing the current, incorrect `cmp x27, x26` sequence. The runtest fails on aarch64 (without `has_lse`); it passes in the interpreter and on the other backends.
The `AtomicCASLoop` sequence compared the loaded value against the expected value with `cmp x27, x26` for `I32`, relying on the `ldaxr` zero-extending the loaded value. However, the upper 32 bits of the register holding the `i32` expected value are unspecified, so if they were nonzero the compare failed even when the low 32 bits matched, and the exchange was not performed. Use the extended-register form `cmp x27, w26, uxtw` for `I32`, as is already done with `uxtb`/`uxth` for `I8`/`I16`.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On aarch64 without LSE,
atomic_cas.i32is lowered toAtomicCASLoop, which compared the loaded value against the expected value with a full 64-bitcmp x27, x26. Theldaxrzero-extends the loaded value, but the upper 32 bits of the register holding thei32expected value are unspecified in the aarch64 backend. For example,ireduce.i32of ani64reuses the same register. When those bits are nonzero, the compare fails even though the low 32 bits match, so the exchange is skipped and the old value is returned. CLIF semantics, the interpreter and the other backends all perform the exchange.I8/I16already compare withuxtb/uxth. This PR does the same forI32and emitscmp x27, w26, uxtw.Commits
runtests/atomic-cas-i32-upper-bits.clif(test interpret+test run, same targets asatomic-cas.clif): usesireduce.i32of ani64with nonzero upper bits as the expected value.isa/aarch64/atomic-cas.clif(precise output), blessed withCRANELIFT_TEST_BLESS=1. It shows the buggycmp x27, x26.inst/emit.rs, which also updates the sequence comment. TheAtomicCASLoopI32encoding inemit_tests.rsis updated, andisa/aarch64/atomic-cas.clifis re-blessed, so both functions now showcmp x27, w26, uxtw.Reduced repro:
Evidence
I cross-built
clif-utilforaarch64-unknown-linux-muslon an x86_64 host at each commit and ran it underqemu-aarch64-static:Commit 1 (tests only):
Commit 2 (fix):
Host (x86_64):
The new runtest passes in the interpreter and on x86_64 both before and after the fix. I checked the riscv64 (
has_a) and s390x lowerings withclif-util compile -D: riscv64 zero-extends both operands beforebne, and s390x usescs, which compares only 32 bits. Neither is affected.