Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary #122220

saethlin · 2024-03-09T02:53:21Z

This special case was added in this PR: #77611 in response to this error message:

Intrinsic has incorrect argument type!
void ({}*)* @llvm.ppc.cfence.p0sl_s
in function rust_oom
LLVM ERROR: Broken function found, compilation aborted!
[RUSTC-TIMING] std test:false 20.161
error: could not compile `std`

But when I tried searching for more information about that intrinsic I found this: llvm/llvm-project#55983 which is a report of someone hitting this same error and a fix was landed in LLVM, 2 years after the above Rust PR.

saethlin · 2024-03-09T15:08:07Z

Seems to work?

saethlin · 2024-03-09T15:12:38Z

@bors rollup=iffy
r? oli-obk
@antoyo I sank some logic from cg_ssa into cg_llvm, so I feel like some change should be made to cg_gcc? Though I don't know to what degree cg_gcc tests AtomicPtr so I don't know if the tests will pass even if this breaks cg_gcc.

antoyo · 2024-03-09T15:45:50Z

I do not know for sure if GCC has the same limitations.
Does your changes require any new tests?

If not, I can just open an issue on the rustc_codegen_gcc repo and link this PR so I could make the change myself if needed.

saethlin · 2024-03-09T15:53:56Z

Does your changes require any new tests?

I'm not sure. AtomicPtr already has some good tests in the library, and I confirmed by trial-and-error that if codegen emits invalid calls to the LLVM atomicrmw intrinsics, they are caught by LLVM assertions. I'm not sure how I'd write a test, since LLVM won't even emit IR if I do the wrong thing.

oli-obk · 2024-03-11T09:49:21Z

compiler/rustc_codegen_llvm/src/builder.rs

        order: rustc_codegen_ssa::common::AtomicOrdering,
    ) -> &'ll Value {
+        // The only RMW operation that LLVM supports on pointers is compare-exchange.


should we report this somewhere upstream? Or at least track it with a FIXME or issue?

Which operations does Rust use on pointers? To the most part these just don't make sense, at least not in a straightforward way (even something like add would run up against the provenance question).

I agree that compare-exchange is kind of the odd-one-out here (in that it makes sense and the other RMW ops do not).

Up to you where the check for it should live 🤷 I decided to sink the branch because otherwise we have an odd interface where we accept an enum but you better only pass one variant.

The check is fine -- my comment was to answer @oli-obk's question on whether this needs to be reported upstream, and I think the answer is "no".

oli-obk · 2024-03-11T09:49:46Z

@bors r+

bors · 2024-03-11T09:49:50Z

📌 Commit aa6cfb2 has been approved by oli-obk

It is now in the queue for this repository.

bors · 2024-03-13T00:03:53Z

⌛ Testing commit aa6cfb2 with merge e61dcc7...

bors · 2024-03-13T02:00:49Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing e61dcc7 to master...

rust-timer · 2024-03-13T05:23:34Z

Finished benchmarking commit (e61dcc7): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

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

Max RSS (memory usage)

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

Cycles

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

Binary size

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

Bootstrap: 672.147s -> 673.298s (0.17%)
Artifact size: 310.26 MiB -> 310.29 MiB (0.01%)

RalfJung · 2024-03-22T07:51:41Z

compiler/rustc_codegen_llvm/src/builder.rs

+        if self.val_ty(src) == self.type_ptr()
+            && op != rustc_codegen_ssa::common::AtomicRmwBinOp::AtomicXchg
+        {
+            src = self.ptrtoint(src, self.type_isize());


If there's a ptrtoint for the argument, doesn't there also have to be an inttoptr for the result?

Hunh. It sure seems like an inttoptr should be required, but the previous implementation didn't have one either, right? I think I'm going to add the conversion back to pointer type anyway with a codegen test.

Add the missing inttoptr when we ptrtoint in ptr atomics Ralf noticed this here: rust-lang#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 ```llvm 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`

Add the missing inttoptr when we ptrtoint in ptr atomics Ralf noticed this here: rust-lang/rust#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 ```llvm 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`

saethlin changed the title ~~Was this just an LLVM bug?~~ PowerPC can into AtomicPtr Mar 9, 2024

saethlin removed A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 9, 2024

This comment has been minimized.

Sign in to view

saethlin force-pushed the ppc-can-into-atomicptr branch from 37cfd25 to 794d604 Compare March 9, 2024 04:08

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 9, 2024

saethlin removed A-testsuite Area: The testsuite used to check the correctness of rustc T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 9, 2024

This comment has been minimized.

Sign in to view

Sink ptrtoint for RMW ops on pointers to cg_llvm

aa6cfb2

saethlin force-pushed the ppc-can-into-atomicptr branch from 794d604 to aa6cfb2 Compare March 9, 2024 15:09

rustbot assigned oli-obk Mar 9, 2024

saethlin marked this pull request as ready for review March 9, 2024 15:12

saethlin changed the title ~~PowerPC can into AtomicPtr~~ Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary Mar 9, 2024

oli-obk reviewed Mar 11, 2024

View reviewed changes

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 Mar 11, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 13, 2024

bors merged commit e61dcc7 into rust-lang:master Mar 13, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Mar 13, 2024

saethlin deleted the ppc-can-into-atomicptr branch March 13, 2024 02:22

oli-obk mentioned this pull request Mar 13, 2024

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

Closed

nikic mentioned this pull request Mar 14, 2024

Compiling std atomic with thread sanitizer causes broken LLVM emission. #122476

Closed

RalfJung reviewed Mar 22, 2024

View reviewed changes

saethlin mentioned this pull request Mar 22, 2024

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary #122220

Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary #122220

saethlin commented Mar 9, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

saethlin commented Mar 9, 2024

saethlin commented Mar 9, 2024

antoyo commented Mar 9, 2024

saethlin commented Mar 9, 2024

oli-obk Mar 11, 2024

nikic Mar 11, 2024

saethlin Mar 11, 2024 •

edited

Loading

nikic Mar 13, 2024

oli-obk commented Mar 11, 2024

bors commented Mar 11, 2024

bors commented Mar 13, 2024

bors commented Mar 13, 2024

rust-timer commented Mar 13, 2024

RalfJung Mar 22, 2024

saethlin Mar 22, 2024 •

edited

Loading

Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary #122220

Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary #122220

Conversation

saethlin commented Mar 9, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

saethlin commented Mar 9, 2024

saethlin commented Mar 9, 2024

antoyo commented Mar 9, 2024

saethlin commented Mar 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saethlin Mar 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Mar 11, 2024

bors commented Mar 11, 2024

bors commented Mar 13, 2024

bors commented Mar 13, 2024

rust-timer commented Mar 13, 2024

Overall result: ✅ improvements - no action needed

Choose a reason for hiding this comment

saethlin Mar 22, 2024 • edited Loading

Choose a reason for hiding this comment

saethlin commented Mar 9, 2024 •

edited

Loading

saethlin Mar 11, 2024 •

edited

Loading

saethlin Mar 22, 2024 •

edited

Loading