Skip to content

Commit 3a6cf27

Browse files
nh13claude
andcommitted
fix(nvvm): keep address-space-qualified pointers out of the generic space
Every access to an `#[address_space(shared)]` (or `constant`) static was emitted as a *generic* access behind an `addrspacecast`, because `load`, `volatile_load`, `gep`, `inbounds_gep` and `check_store` all cast their pointer operand through `type_ptr_to`, which always yields an addrspace 0 pointer. Correctness then rests entirely on libNVVM's `InferAddressSpaces` pass folding the cast back into the access. It usually does, which is why this mostly works. When it does not, the access is emitted as a generic `ld`/`st` against the raw shared window offset -- an out-of-bounds access at runtime, with no error or warning at compile time, and only on the warps whose data reaches the affected branch. Cast the pointer operand in the address space it already points into instead. For the kernel added in this commit the emitted NVVM IR goes from %22 = getelementptr inbounds i32, i32* addrspacecast (i32 addrspace(3)* ... @sh ... to i32*), i64 %18 store i32 %1, i32* %22, align 4 to %22 = getelementptr inbounds i32, i32 addrspace(3)* ... @sh ..., i64 %18 store i32 %1, i32 addrspace(3)* %22, align 4 `raw_eq`'s integer-compare fast path had the same defect and is fixed with it. Casts that are *semantically* required -- a pointer passed to a callee whose parameter is generic, or to `memcmp` -- are left alone; those still go through `bitcast`, which addrspacecasts to generic as before. Add a `dis` compiletest pinning the shared-memory codegen shape: the kernel must use `st.shared`/`ld.shared` on the raw symbol offset and needs no `cvta.shared`. Refs #402 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 103a8d5 commit 3a6cf27

4 files changed

Lines changed: 105 additions & 12 deletions

File tree

crates/rustc_codegen_nvvm/src/builder.rs

Lines changed: 38 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -509,7 +509,7 @@ impl<'ll, 'tcx, 'a> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
509509

510510
fn load(&mut self, ty: &'ll Type, ptr: &'ll Value, align: Align) -> &'ll Value {
511511
trace!("Load {ty:?} {:?}", ptr);
512-
let ptr = self.pointercast(ptr, self.cx.type_ptr_to(ty));
512+
let ptr = self.pointercast_preserving_addrspace(ptr, ty);
513513
unsafe {
514514
#[cfg(feature = "llvm19")]
515515
let load = llvm::LLVMBuildLoad2(self.llbuilder, ty, ptr, UNNAMED);
@@ -522,7 +522,7 @@ impl<'ll, 'tcx, 'a> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
522522

523523
fn volatile_load(&mut self, ty: &'ll Type, ptr: &'ll Value) -> &'ll Value {
524524
trace!("Volatile load `{:?}`", ptr);
525-
let ptr = self.pointercast(ptr, self.cx.type_ptr_to(ty));
525+
let ptr = self.pointercast_preserving_addrspace(ptr, ty);
526526
unsafe {
527527
#[cfg(feature = "llvm19")]
528528
let load = llvm::LLVMBuildLoad2(self.llbuilder, ty, ptr, UNNAMED);
@@ -794,7 +794,7 @@ impl<'ll, 'tcx, 'a> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
794794

795795
fn gep(&mut self, ty: &'ll Type, ptr: &'ll Value, indices: &[&'ll Value]) -> &'ll Value {
796796
trace!("gep: {ty:?} {:?} with indices {:?}", ptr, indices);
797-
let ptr = self.pointercast(ptr, self.cx().type_ptr_to(ty));
797+
let ptr = self.pointercast_preserving_addrspace(ptr, ty);
798798
unsafe {
799799
llvm::LLVMBuildGEP2(
800800
self.llbuilder,
@@ -814,7 +814,7 @@ impl<'ll, 'tcx, 'a> BuilderMethods<'a, 'tcx> for Builder<'a, 'll, 'tcx> {
814814
indices: &[&'ll Value],
815815
) -> &'ll Value {
816816
trace!("gep inbounds: {ty:?} {:?} with indices {:?}", ptr, indices);
817-
let ptr = self.pointercast(ptr, self.cx().type_ptr_to(ty));
817+
let ptr = self.pointercast_preserving_addrspace(ptr, ty);
818818
unsafe {
819819
llvm::LLVMBuildInBoundsGEP2(
820820
self.llbuilder,
@@ -1497,17 +1497,35 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> {
14971497

14981498
fn noundef_metadata(&mut self, _load: &'ll Value) {}
14991499

1500+
/// Casts `ptr` to a pointer to `ty`, leaving it in the address space it already points into.
1501+
///
1502+
/// Use this rather than casting to `type_ptr_to(ty)` for the pointer operand of a GEP or of
1503+
/// a memory access. `type_ptr_to` always yields a generic (addrspace 0) pointer, so casting
1504+
/// through it turns every access to an `#[address_space(shared)]` (or `constant`) static
1505+
/// into a generic access behind an `addrspacecast`. libNVVM's `InferAddressSpaces` pass
1506+
/// usually folds those back into `ld.shared`/`st.shared`, but it is not obliged to, and
1507+
/// when it misses one the access is emitted as a generic `ld`/`st` against the raw shared
1508+
/// window offset -- an out-of-bounds access at runtime, with no diagnostic at compile time.
1509+
pub(crate) fn pointercast_preserving_addrspace(
1510+
&mut self,
1511+
ptr: &'ll Value,
1512+
ty: &'ll Type,
1513+
) -> &'ll Value {
1514+
let addrspace = pointer_addrspace(self.cx, ptr);
1515+
self.pointercast(ptr, self.cx.type_ptr_to_ext(ty, addrspace))
1516+
}
1517+
15001518
fn check_store(&mut self, val: &'ll Value, ptr: &'ll Value) -> &'ll Value {
15011519
let dest_ptr_ty = self.cx.val_ty(ptr);
15021520
let stored_ty = self.cx.val_ty(val);
1503-
let stored_ptr_ty = self.cx.type_ptr_to(stored_ty);
1504-
1505-
assert_eq!(self.cx.type_kind(dest_ptr_ty), TypeKind::Pointer);
1521+
let stored_ptr_ty = self
1522+
.cx
1523+
.type_ptr_to_ext(stored_ty, pointer_addrspace(self.cx, ptr));
15061524

15071525
if dest_ptr_ty == stored_ptr_ty {
15081526
ptr
15091527
} else {
1510-
self.bitcast(ptr, stored_ptr_ty)
1528+
self.pointercast(ptr, stored_ptr_ty)
15111529
}
15121530
}
15131531

@@ -1601,7 +1619,11 @@ impl<'a, 'll, 'tcx> Builder<'a, 'll, 'tcx> {
16011619
impl<'ll, 'tcx, 'a> Builder<'a, 'll, 'tcx> {
16021620
/// Implements a standard atomic, using LLVM intrinsics(in `atomic_supported`, if `dst` is in a supported address space)
16031621
/// or emulation(with `emulate_local`, if `dst` points to a thread-local address space).
1604-
/// FIXME(FractalFir): this code assumess all pointers are generic. Adjust it once we support address spaces.
1622+
/// `dst` may already be in a specific address space, in which case the `isspacep` checks
1623+
/// below are redundant: `check_call` casts them to generic for the predicate calls, and the
1624+
/// atomic itself stays on the original pointer.
1625+
/// FIXME(FractalFir): skip the runtime predicate dance entirely when the address space of
1626+
/// `dst` is statically known.
16051627
fn atomic_op(
16061628
&mut self,
16071629
dst: &'ll Value,
@@ -1694,3 +1716,10 @@ impl<'ll, 'tcx, 'a> Builder<'a, 'll, 'tcx> {
16941716
)
16951717
}
16961718
}
1719+
1720+
/// The address space `ptr` points into.
1721+
fn pointer_addrspace<'ll>(cx: &CodegenCx<'ll, '_>, ptr: &'ll Value) -> AddressSpace {
1722+
let ty = cx.val_ty(ptr);
1723+
assert_eq!(cx.type_kind(ty), TypeKind::Pointer);
1724+
unsafe { AddressSpace(llvm::LLVMGetPointerAddressSpace(ty)) }
1725+
}

crates/rustc_codegen_nvvm/src/intrinsic.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -705,10 +705,11 @@ impl<'ll, 'tcx> IntrinsicCallBuilderMethods<'tcx> for Builder<'_, 'll, 'tcx> {
705705
self.const_bool(true)
706706
} else if use_integer_compare {
707707
let integer_ty = self.type_ix(layout.size.bits());
708-
let ptr_ty = self.type_ptr_to(integer_ty);
709-
let a_ptr = self.bitcast(a, ptr_ty);
708+
// Keep both operands in their own address space; casting to a generic
709+
// pointer here would make the loads below generic accesses.
710+
let a_ptr = self.pointercast_preserving_addrspace(a, integer_ty);
710711
let a_val = self.load(integer_ty, a_ptr, layout.align.abi);
711-
let b_ptr = self.bitcast(b, ptr_ty);
712+
let b_ptr = self.pointercast_preserving_addrspace(b, integer_ty);
712713
let b_val = self.load(integer_ty, b_ptr, layout.align.abi);
713714
self.icmp(IntPredicate::IntEQ, a_val, b_val)
714715
} else {
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// build-pass
2+
// compile-flags: -Cllvm-args=--disassemble-entry=shared_broadcast --error-format=human -Cdebuginfo=0
3+
4+
// Regression test: accesses to an `#[address_space(shared)]` static must be emitted in the
5+
// shared address space, not as generic accesses behind an `addrspacecast` to addrspace 0.
6+
//
7+
// The kernel below must use `st.shared`/`ld.shared` on the raw symbol offset, and must contain
8+
// no `cvta.shared` -- there is nothing here that needs a generic pointer.
9+
10+
use core::mem::MaybeUninit;
11+
use cuda_std::{address_space, kernel, thread};
12+
13+
#[address_space(shared)]
14+
static mut SH: [MaybeUninit<u32>; 32] = [MaybeUninit::uninit(); 32];
15+
16+
#[kernel]
17+
pub unsafe fn shared_broadcast(out: *mut u32, n: u32) {
18+
let lane = thread::thread_idx_x() & 31;
19+
let warp = ((thread::thread_idx_x() >> 5) & 31) as usize;
20+
unsafe {
21+
if lane == 0 {
22+
SH[warp] = MaybeUninit::new(n);
23+
}
24+
thread::sync_threads();
25+
*out = SH[warp].assume_init();
26+
}
27+
}
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
.visible .entry shared_broadcast(
2+
.param .u64 shared_broadcast_param_0,
3+
.param .u32 shared_broadcast_param_1
4+
)
5+
{
6+
.reg .pred %p<2>;
7+
.reg .b32 %r<6>;
8+
.reg .b64 %rd<6>;
9+
// demoted variable
10+
.shared .align 4 .b8 _RNvCs6I4kclYupDO_13shared_memory2SH[128];
11+
12+
ld.param.u64 %rd2, [shared_broadcast_param_0];
13+
ld.param.u32 %r1, [shared_broadcast_param_1];
14+
mov.u32 %r2, %tid.x;
15+
and.b32 %r3, %r2, 31;
16+
shr.u32 %r4, %r2, 5;
17+
setp.eq.s32 %p1, %r3, 0;
18+
mul.wide.u32 %rd3, %r4, 4;
19+
mov.u64 %rd4, _RNvCs6I4kclYupDO_13shared_memory2SH;
20+
add.s64 %rd1, %rd4, %rd3;
21+
@%p1 bra $L__BB0_1;
22+
bra.uni $L__BB0_2;
23+
24+
$L__BB0_1:
25+
st.shared.u32 [%rd1], %r1;
26+
27+
$L__BB0_2:
28+
bar.sync 0;
29+
ld.shared.u32 %r5, [%rd1];
30+
cvta.to.global.u64 %rd5, %rd2;
31+
st.global.u32 [%rd5], %r5;
32+
ret;
33+
34+
}
35+
36+

0 commit comments

Comments
 (0)