fix(nvvm): cast bit-counting intrinsic results back to the return width#403
Open
nh13 wants to merge 1 commit into
Open
fix(nvvm): cast bit-counting intrinsic results back to the return width#403nh13 wants to merge 1 commit into
nh13 wants to merge 1 commit into
Conversation
`core::intrinsics::{ctpop, ctlz, cttz, ctlz_nonzero, cttz_nonzero}` are
declared as returning `u32` for every operand width, while the LLVM
intrinsics they lower to return a value as wide as their operand.
`rustc_codegen_llvm` casts the result back; this backend did not.
The oversized value was then stored into the 4-byte result slot. For
`u64`/`u128` that store is out of bounds for the slot and is discarded, the
intrinsic call becomes dead, and the device silently computes `0` -- the
emitted PTX contains no `popc`/`clz` at all. For `u8`/`u16` the store is in
bounds, so the high bytes of the result are left uninitialized. Only width
32 was correct, because there the cast is a no-op.
Wrap the whole arm in a single `intcast` to the intrinsic's result layout
rather than casting in each of the five arms. Casting to an identical type
is a no-op, so the intrinsics in this arm that do return the operand width
(`bswap`, `bitreverse`, `rotate_*`, `saturating_*`) are unaffected, and any
intrinsic added here later is covered automatically.
Add `dis` compiletests pinning the emitted PTX for `u64::count_ones`
(`popc.b64`), `u64::leading_zeros` (`clz.b64`), and the emulated
`u128::count_ones` (two `popc.b64` plus the combining `add`). Before this
change all three kernels compiled down to a bare `ret`.
Also correct the 128-bit row in the feature table, which claimed the
bit-manipulation intrinsics were unsupported.
Fixes Rust-GPU#401
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Fixes #401.
core::intrinsics::{ctpop, ctlz, cttz, ctlz_nonzero, cttz_nonzero}are declared as returningu32for every operand width, while the LLVM intrinsics they lower to return a value as wide as their operand.rustc_codegen_llvmcasts the result back (theintcast(ret, result.layout.llvm_type(self), false)in each arm ofcompiler/rustc_codegen_llvm/src/intrinsic.rs); this backend never has.The oversized value was then stored into the 4-byte result slot. For
u64/u128that store is out of bounds for the slot, so it is discarded, the intrinsic call becomes dead, and the device silently computes0. Foru8/u16the store is in bounds and the high bytes of the result are left uninitialized. Width 32 was the only correct width, because there the cast is a no-op.This is easy to see in the emitted PTX. Before this change, the entire body of
is gone -- no
popc, and not even the store:After, the
popc.b64is there and the result is stored, and the resulting SASS is a correct 64-bit population count (POPC/POPC/IADD3/STG.E).The fix
Rather than wrapping each of the five arms individually (and
handle_128_bit_intrinsic, which is a free function with noresultin scope), the whole arm is wrapped in a singleintcastto the intrinsic's result layout. Casting to an identical type is a no-op, so the intrinsics in that arm which do return the operand width --bswap,bitreverse,rotate_*,saturating_*-- are unaffected, and any intrinsic added there later is covered automatically.Tests
Three
discompiletests pin the emitted PTX:count_ones_u64-- must containpopc.b64leading_zeros_u64-- must containclz.b64count_ones_u128-- exercises the separate emulatedhandle_128_bit_intrinsicpath; must contain twopopc.b64plus the combiningaddAll three compiled down to a bare
retbefore this change. They pass-Cdebuginfo=0so the goldens do not embedlibrary/coreline numbers, which would otherwise churn on everyrust-toolchain.tomlbump.Verified in
ghcr.io/rust-gpu/rust-cuda-ubuntu24-cuda12: full compiletest suite green acrosscompute_61,compute_75,compute_90(66 passed), andcargo fmt --all -- --checkpluscargo clippy -p rustc_codegen_nvvmclean withRUSTFLAGS=-Dwarnings.Also corrects the 128-bit row in
guide/src/features.md, which claimed the bit-manipulation intrinsics were unsupported -- they are emulated over the two 64-bit halves, and with this fix they return correct results.🤖 Generated with Claude Code