Skip to content

Conversation

@Enselic
Copy link
Member

@Enselic Enselic commented Dec 4, 2025

Closes #60044 which has one 👍 and one ❤️ vote and just E-needs-test.

@rustbot 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 Dec 4, 2025
@rustbot
Copy link
Collaborator

rustbot commented Dec 4, 2025

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@clubby789
Copy link
Contributor

I think this should be a codegen-llvm test as it's not backend-specific

@Enselic
Copy link
Member Author

Enselic commented Dec 9, 2025

Good call. That deserves consideration.

Here is what the correct LLVM IR currently looks like:

; Function Attrs: mustprogress nofree norecurse nounwind nonlazybind willreturn memory(readwrite, argmem: none, inaccessiblemem: write)
define noundef range(i64 1, 0) i64 @some_non_zero_from_atomic_get() unnamed_addr #0 {
start:
  %0 = load atomic i64, ptr @_ZN38some_non_zero_from_atomic_optimization1X17h41fcdb7c72ef9763E monotonic, align 8
  %1 = icmp ne i64 %0, 0
  tail call void @llvm.assume(i1 %1)
  ret i64 %0
}
Full LLVM IR
; ModuleID = 'some_non_zero_from_atomic_optimization.ca4055969c8517ab-cgu.0'
source_filename = "some_non_zero_from_atomic_optimization.ca4055969c8517ab-cgu.0"
target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

@_ZN38some_non_zero_from_atomic_optimization1X17h41fcdb7c72ef9763E = local_unnamed_addr global [8 x i8] c"\07\00\00\00\00\00\00\00", align 8

@some_non_zero_from_atomic_get2 = unnamed_addr alias i64 (), ptr @some_non_zero_from_atomic_get

; Function Attrs: mustprogress nofree norecurse nounwind nonlazybind willreturn memory(readwrite, argmem: none, inaccessiblemem: write)
define noundef range(i64 1, 0) i64 @some_non_zero_from_atomic_get() unnamed_addr #0 {
start:
  %0 = load atomic i64, ptr @_ZN38some_non_zero_from_atomic_optimization1X17h41fcdb7c72ef9763E monotonic, align 8
  %1 = icmp ne i64 %0, 0
  tail call void @llvm.assume(i1 %1)
  ret i64 %0
}

; Function Attrs: mustprogress nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: write)
declare void @llvm.assume(i1 noundef) #1

attributes #0 = { mustprogress nofree norecurse nounwind nonlazybind willreturn memory(readwrite, argmem: none, inaccessiblemem: write) "probe-stack"="inline-asm" "target-cpu"="x86-64" }
attributes #1 = { mustprogress nocallback nofree nosync nounwind willreturn memory(inaccessiblemem: write) }

!llvm.module.flags = !{!0, !1}
!llvm.ident = !{!2}

!0 = !{i32 8, !"PIC Level", i32 2}
!1 = !{i32 2, !"RtLibUseGOT", i32 1}
!2 = !{!"rustc version 1.91.1 (ed61e7d7e 2025-11-07)"}

As you can see, the LLVM IR is considerably more elaborate and is likely to change over time as we adjust codegen and adapt to new LLVM versions. So an LLVM IR version of this test risks becoming a maintenance burden that will steal time from doing other improvements.

Besides, it does not seem impossible that this optimization could regress on the LLVM side, which an LLVM IR based test would not catch.

So my recommendation is to go ahead with this test, because it tests what actually matters (the end result) and that is unlikely to ever change.

That said, I can certainly turn this into an LLVM test if that is considered mandatory for merge. We can wait and see see what others think perhaps.

@Mark-Simulacrum
Copy link
Member

@bors r+

I don't think it has to check the LLVM IR necessarily.

@bors
Copy link
Collaborator

bors commented Dec 20, 2025

📌 Commit faf06cc has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors 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 Dec 20, 2025
Zalathar added a commit to Zalathar/rust that referenced this pull request Dec 21, 2025
…acrum

tests/assembly-llvm/some-non-zero-from-atomic-optimization.rs: New test

Closes rust-lang#60044 which has one 👍 and one ❤️  vote and just **E-needs-test**.
bors added a commit that referenced this pull request Dec 21, 2025
Rollup of 13 pull requests

Successful merges:

 - #146377 (Don't strip shebang in expr-ctxt `include!(…)`)
 - #149437 (Fix trailing newline in JUnit formatter)
 - #149658 (tests/assembly-llvm/some-non-zero-from-atomic-optimization.rs: New test)
 - #149812 (Add const default for OnceCell and OnceLock)
 - #149882 (miri: add -Zbinary-dep-depinfo to dependency builds)
 - #150009 (Enable llvm-libunwind by default for Hexagon targets)
 - #150035 (fix docustring on fetch_or)
 - #150082 (tests/ui/traits/fmt-pointer-trait.rs: Add HRTB fn pointer case)
 - #150160 (Fix ICE (#149980) for invalid EII in statement position)
 - #150184 (mir_build: Use the same length type for `TestableCase::Slice` and `TestKind::Len`)
 - #150191 (change non-canonical clone impl to {*self}, fix some doc comments)
 - #150203 (Drop the From derive macro from the v1 prelude)
 - #150208 (Update books)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 21, 2025
…acrum

tests/assembly-llvm/some-non-zero-from-atomic-optimization.rs: New test

Closes rust-lang#60044 which has one 👍 and one ❤️  vote and just **E-needs-test**.
bors added a commit that referenced this pull request Dec 21, 2025
Rollup of 11 pull requests

Successful merges:

 - #146377 (Don't strip shebang in expr-ctxt `include!(…)`)
 - #149658 (tests/assembly-llvm/some-non-zero-from-atomic-optimization.rs: New test)
 - #149812 (Add const default for OnceCell and OnceLock)
 - #149882 (miri: add -Zbinary-dep-depinfo to dependency builds)
 - #150009 (Enable llvm-libunwind by default for Hexagon targets)
 - #150035 (fix docustring on fetch_or)
 - #150082 (tests/ui/traits/fmt-pointer-trait.rs: Add HRTB fn pointer case)
 - #150160 (Fix ICE (#149980) for invalid EII in statement position)
 - #150184 (mir_build: Use the same length type for `TestableCase::Slice` and `TestKind::Len`)
 - #150191 (change non-canonical clone impl to {*self}, fix some doc comments)
 - #150203 (Drop the From derive macro from the v1 prelude)

r? `@ghost`
`@rustbot` modify labels: rollup
@JonathanBrouwer
Copy link
Contributor

@bors r- #150220 (comment)

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 21, 2025
@clubby789
Copy link
Contributor

.set some_non_zero_from_atomic_get2, some_non_zero_from_atomic_get 

vs

// CHECK-DAG: some_non_zero_from_atomic_get2 = some_non_zero_from_atomic_get

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missed NonZero optimization opportunity

6 participants