-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Directly use raw pointers in AtomicPtr
store/load
#77611
Conversation
r? @sfackler (rust_highfive has picked a reviewer for you, use r? to override) |
9d1626f
to
2da5960
Compare
With this you can probably remove the use of |
Removed that and miri still tests successfully |
r? @nagisa (I don't know who else to ping for llvm stuff) |
AFAIK @cuviper is also knowledgeable for matters of LLVM |
There's an entire workgroup of us! (rust-lang/wg-llvm) This seems just fine to me, @bors r+ rollup=iffy |
📌 Commit 31c1dfbe12870cea1144b5d0c77785acc1b4ce70 has been approved by |
⌛ Testing commit 31c1dfbe12870cea1144b5d0c77785acc1b4ce70 with merge 3610b9b4bb45c31e2e7af4814c95fe2106738100... |
💔 Test failed - checks-actions |
😆 nevermind, we found a platform where this doesn't work
I'll adjust the llvm backend to do the appropriate transformation instead of having that in libcore |
@bors r- |
Oh, this looks great, I feared it would be much more complicated. :) If I understood correctly, since this happens in the general SSA backend, the Cranelift backend will not need to do anything itself? Cc @bjorn3 |
I don't use cg_ssa yet. Cranelift doesn't have a separate pointer type, so no changes to the actual codegen are necessary. There is a validation for the used type though. The fix for cg_clif should be a matter of adding
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cg_clif change LGTM
// SAFETY: This intrinsic is unsafe because it operates on a raw pointer | ||
// but we know for sure that the pointer is valid (we just got it from | ||
// an `UnsafeCell` that we have by reference) and the atomic operation | ||
// itself allows us to safely mutate the `UnsafeCell` contents. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't match the safety comment for the #[cfg(bootstrap)]
case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, but I felt like it wasn't worth updating that, too, since it'll go away with the next beta anyway
@bors r+ rollup=never |
📌 Commit 41f988e has been approved by |
⌛ Testing commit 41f988e with merge 0bddb8da0ff78a761169b9ac1b18cde962fb88f6... |
💔 Test failed - checks-actions |
It says "This check failed" without any logs. I guess it is spurious @bors retry |
☀️ Test successful - checks-actions |
Directly use raw pointers in `AtomicPtr` store/load I was unable to find any reason for this limitation in the latest source of LLVM or in the documentation [here](http://llvm.org/docs/Atomics.html#libcalls-atomic). fixes rust-lang/miri#1574
…li-obk Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary This special case was added in this PR: rust-lang#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.
Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary This special case was added in this PR: rust-lang/rust#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.
Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary This special case was added in this PR: rust-lang/rust#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.
Only generate a ptrtoint in AtomicPtr codegen when absolutely necessary This special case was added in this PR: rust-lang/rust#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.
I was unable to find any reason for this limitation in the latest source of LLVM or in the documentation here.
fixes rust-lang/miri#1574