Add the missing inttoptr when we ptrtoint in ptr atomics #122917

saethlin · 2024-03-22T22:28:59Z

Ralf noticed this here: #122220 (comment)

Our previous codegen forgot to add the cast back to integer type. The code compiles anyway, because of course all locals are in-memory to start with, so previous codegen would do the integer atomic, store the integer to a local, then load a pointer from that local. Which is definitely not what we wanted: That's an integer-to-pointer transmute, so all pointers returned by these AtomicPtr methods didn't have provenance. Yikes.

Here's the IR for AtomicPtr::fetch_byte_add on 1.76: https://godbolt.org/z/8qTEjeraY

define noundef ptr @atomicptr_fetch_byte_add(ptr noundef nonnull align 8 %a, i64 noundef %v) unnamed_addr #0 !dbg !7 {
start:
  %0 = alloca ptr, align 8, !dbg !12
  %val = inttoptr i64 %v to ptr, !dbg !12
  call void @llvm.lifetime.start.p0(i64 8, ptr %0), !dbg !28
  %1 = ptrtoint ptr %val to i64, !dbg !28
  %2 = atomicrmw add ptr %a, i64 %1 monotonic, align 8, !dbg !28
  store i64 %2, ptr %0, align 8, !dbg !28
  %self = load ptr, ptr %0, align 8, !dbg !28
  call void @llvm.lifetime.end.p0(i64 8, ptr %0), !dbg !28
  ret ptr %self, !dbg !33
}

r? @RalfJung
cc @nikic

erikdesjardins · 2024-03-23T01:24:54Z

tests/codegen/atomicptr.rs

+    // CHECK: %[[INTPTR:.*]] = ptrtoint ptr %{{.*}} to [[USIZE]]
+    // CHECK-NEXT: %[[RET:.*]] = atomicrmw add ptr %{{.*}}, [[USIZE]] %[[INTPTR]]
+    // CHECK-NEXT: inttoptr [[USIZE]] %[[RET]] to ptr


I think this would be clearer (and not too fragile) if you match the whole thing end-to-end, i.e.

Suggested change

// CHECK: %[[INTPTR:.*]] = ptrtoint ptr %{{.*}} to [[USIZE]]

// CHECK-NEXT: %[[RET:.*]] = atomicrmw add ptr %{{.*}}, [[USIZE]] %[[INTPTR]]

// CHECK-NEXT: inttoptr [[USIZE]] %[[RET]] to ptr

// CHECK: %[[V_PTR:.*]] = inttoptr [[USIZE]] %v to ptr

// CHECK: %[[V_INT:.*]] = ptrtoint ptr %[[V_PTR]] to [[USIZE]]

// CHECK: %[[RET_INT:.*]] = atomicrmw add ptr %a, [[USIZE]] %[[V_INT]]

// CHECK: %[[RET_PTR:.*]] = inttoptr [[USIZE]] %[[RET]] to ptr

// CHECK: ret ptr %[[RET_PTR]]

(Could also match the argument names from the signature instead of hardcoding %a and %v, but I think it's fine, it's pretty unlikely that LLVM will start changing/dropping them in the future.)

This then leads to the question--why do we roundtrip %v through inttoptr/ptrtoint?

I suppose this is because in MIR the atomic op takes two pointer arguments, but does it really make sense to add two pointers?

Correct, adding two pointers doesn't make sense. The problem is that we're implementing an operation which takes a pointer operand and an integer operand. There's no atomic ptradd.

In terms of provenance, this is cursed if there are two pointer operands because which provenance is written back through the pointer? It's also cursed if we do this with integers, because then haven't we written an integer over a pointer, wiping the provenance from the memory? (I'm sort of hand-waving the kind of provenance semantics in Miri over LLVM IR which is itself a bit dubious)

Right, that makes sense. I should've been more explicit--what I mean is, the relevant part of the MIR for your example currently looks like:

fn atomicptr_fetch_byte_add(_1: &AtomicPtr<u8>, _2: usize) -> *mut u8 { ... _4 = move _7 as *mut *mut u8 (PtrToPtr); _6 = _2 as *mut u8 (Transmute); _3 = atomic_xadd_relaxed::<*mut u8>(move _4, move _6) -> [return: bb1, unwind unreachable];

i.e. the intrinsic we're implementing here does take two pointer operands. Isn't that wrong? (Well, maybe not wrong but suboptimal.)

In what way would an intrinsic based on a pointer to an integer and and an integer operand be better? Or are you suggesting that we add an intrinsic fn atomic_ptradd_relaxed(*mut *mut T, usize)?

Or are you suggesting that we add an intrinsic fn atomic_ptradd_relaxed(*mut *mut T, usize)?

Yes. (And the same for every atomic operation on pointers (xor, or, etc.) that only makes sense with an integer second operand.)

Given that the only sensible operation here is offsetting a pointer by an integer, wouldn't it be better for the intrinsic to match that?

(I guess this isn't so much a problem because we can just define atomic_xadd_relaxed::<*mut u8> to always use the provenance of the first argument, and I assume this is what Miri implements. Having to convert the offset to/from a pointer "just" makes the codegen more complex.)

always use the provenance of the first argument, and I assume this is what Miri implements.

Yes indeed.

But ideally we'd convince LLVM to provide an intrinsic that takes (*mut *mut T, usize); that is the only way to avoid pointer-integer transmutation.

Right, we need the LLVM change regardless, I don't mean to imply that this would solve the provenance issue. It would just remove some other unnecessary weirdness.

It doesn't need to be done as part of this PR though, I can do it separately.

Agree with @erikdesjardins that the Rust intrinsic should accept usize rather than pointer for the second arg.

erikdesjardins · 2024-03-23T01:31:22Z

tests/codegen/atomicptr.rs

+
+// CHECK-LABEL: @atomicptr_swap
+#[no_mangle]
+pub fn atomicptr_swap(a: &AtomicPtr<u8>, ptr1: *mut u8, ptr2: *mut u8, ptr3: *mut u8) -> *mut u8 {


Are the extra arguments necessary?

Whoops. Leftover from a previous idea.

RalfJung · 2024-03-23T09:34:55Z

Our previous codegen forgot to add the cast back to integer type. The code compiles anyway, because of course all locals are in-memory to start with, so previous codegen would do the integer atomic, store the integer to a local, then load a pointer from that local. Which is definitely not what we wanted: That's an integer-to-pointer transmute, so all pointers returned by these AtomicPtr methods didn't have provenance. Yikes.

This assumes LLVM uses our semantics for int2ptr transmutes, which is far from given. But it'd be a conservative choice to not rely on these transmutes to work.

OTOH, the transmutes still occur with this PR if load/store/CAS and fetch_add are mixed on the same AtomicPtr. The only proper fix is to get atomicrmw add of a pointer and an integer into LLVM, where the resulting pointer obviously has the provenance of the only pointer argument. @nikic do you think that is something LLVM would be receptive to?

Meanwhile, the question here is what is the way to generate code that is least likely to make LLVM do anything wrong. That's outside my league.
r? @nikic

nikic · 2024-04-15T05:19:34Z

@bors r+

bors · 2024-04-15T05:19:37Z

📌 Commit 6b794f6 has been approved by nikic

It is now in the queue for this repository.

nikic · 2024-04-15T05:22:59Z

OTOH, the transmutes still occur with this PR if load/store/CAS and fetch_add are mixed on the same AtomicPtr. The only proper fix is to get atomicrmw add of a pointer and an integer into LLVM, where the resulting pointer obviously has the provenance of the only pointer argument. @nikic do you think that is something LLVM would be receptive to?

I'd say basically "yes". But at the same time I don't think this particularly matters right now, given how atomicrmw operates in-memory, so the actual type of the value is hidden. I guess the only case where this would become visible is if LLVM expands the atomicrmw into a cmpxchg loop.

As such I'd probably delay such an addition until we have ptradd, in which case atomicrmw ptradd would be an obvious extension.

bors · 2024-04-15T08:07:51Z

⌛ Testing commit 6b794f6 with merge 5dcb678...

bors · 2024-04-15T10:10:39Z

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 5dcb678 to master...

rust-timer · 2024-04-15T11:26:29Z

Finished benchmarking commit (5dcb678): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.4%	[2.4%, 2.4%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.4%	[2.4%, 2.4%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	6.3%	[2.8%, 9.7%]	2
Regressions ❌ (secondary)	2.2%	[2.2%, 2.2%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	6.3%	[2.8%, 9.7%]	2

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 677.003s -> 679.002s (0.30%)
Artifact size: 316.05 MiB -> 316.01 MiB (-0.01%)

rustbot assigned RalfJung Mar 22, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 22, 2024

saethlin marked this pull request as draft March 22, 2024 22:35

saethlin marked this pull request as ready for review March 22, 2024 23:03

erikdesjardins reviewed Mar 23, 2024

View reviewed changes

Add the missing inttoptr when we ptrtoint in ptr atomics

6b794f6

saethlin force-pushed the atomicptr-to-int branch from 76fa2d4 to 6b794f6 Compare March 23, 2024 04:07

rustbot assigned nikic and unassigned RalfJung Mar 23, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 15, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 15, 2024

bors merged commit 5dcb678 into rust-lang:master Apr 15, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Apr 15, 2024

bors mentioned this pull request Apr 15, 2024

Assert that we only use compare-exchange on pointers and no other rmw op #122428

Closed

saethlin deleted the atomicptr-to-int branch May 3, 2024 22:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the missing inttoptr when we ptrtoint in ptr atomics #122917

Add the missing inttoptr when we ptrtoint in ptr atomics #122917

saethlin commented Mar 22, 2024 •

edited

Loading

erikdesjardins Mar 23, 2024

erikdesjardins Mar 23, 2024

saethlin Mar 23, 2024

erikdesjardins Mar 23, 2024 •

edited

Loading

saethlin Mar 23, 2024

erikdesjardins Mar 23, 2024 •

edited

Loading

RalfJung Mar 23, 2024

erikdesjardins Mar 24, 2024 •

edited

Loading

nikic Apr 15, 2024

erikdesjardins Mar 23, 2024

saethlin Mar 23, 2024

RalfJung commented Mar 23, 2024 •

edited

Loading

nikic commented Apr 15, 2024

bors commented Apr 15, 2024

nikic commented Apr 15, 2024

bors commented Apr 15, 2024

bors commented Apr 15, 2024

rust-timer commented Apr 15, 2024

-    // CHECK: %[[INTPTR:.*]] = ptrtoint ptr %{{.*}} to [[USIZE]]
-    // CHECK-NEXT: %[[RET:.*]] = atomicrmw add ptr %{{.*}}, [[USIZE]] %[[INTPTR]]
-    // CHECK-NEXT: inttoptr [[USIZE]] %[[RET]] to ptr
+    // CHECK: %[[V_PTR:.*]] = inttoptr [[USIZE]] %v to ptr
+    // CHECK: %[[V_INT:.*]] = ptrtoint ptr %[[V_PTR]] to [[USIZE]]
+    // CHECK: %[[RET_INT:.*]] = atomicrmw add ptr %a, [[USIZE]] %[[V_INT]]
+    // CHECK: %[[RET_PTR:.*]] = inttoptr [[USIZE]] %[[RET]] to ptr
+    // CHECK: ret ptr %[[RET_PTR]]

Add the missing inttoptr when we ptrtoint in ptr atomics #122917

Add the missing inttoptr when we ptrtoint in ptr atomics #122917

Conversation

saethlin commented Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdesjardins Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdesjardins Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikdesjardins Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Mar 23, 2024 • edited Loading

nikic commented Apr 15, 2024

bors commented Apr 15, 2024

nikic commented Apr 15, 2024

bors commented Apr 15, 2024

bors commented Apr 15, 2024

rust-timer commented Apr 15, 2024

Overall result: no relevant changes - no action needed

saethlin commented Mar 22, 2024 •

edited

Loading

erikdesjardins Mar 23, 2024 •

edited

Loading

erikdesjardins Mar 23, 2024 •

edited

Loading

erikdesjardins Mar 24, 2024 •

edited

Loading

RalfJung commented Mar 23, 2024 •

edited

Loading