Add element-wise atomic memory operations #59155

tmccombs · 2019-03-13T07:57:44Z

WIP

This is an attempt to implement #58599

rust-highfive · 2019-03-13T07:57:54Z

(rust_highfive has picked a reviewer for you, use r? to override)

tmccombs · 2019-03-13T08:03:43Z

src/librustc_codegen_llvm/intrinsic.rs

@@ -549,6 +553,17 @@ impl IntrinsicCallMethods<'tcx> for Builder<'a, 'll, 'tcx> {
                    Err(()) => return
                }
            }
+            name if name.starts_with("atomic_element_") => {
+                let ty = substs.type_at(0);
+                if int_type_width_signed(ty, self).is_some() {


This restricts the type to integer types. I'm not sure if this is the best action, or if we should accept any type with a size that is a power of two less than the target-specific atomic size limit. But this is what we do for other atomic operations, so it is my first inclination.

tmccombs · 2019-03-13T08:05:20Z

src/rustllvm/RustWrapper.cpp

+      unwrap(Size), ElementSize));
+#else
+  // TODO
+  abort();


LLVM 6.0 doesn't have a method for the memmove or memcpy intrinsics.

Could this and the version below return a nullptr in this case and then it's handled in rustc with a bug/panic macro?

that's a possibility. Since LLVM 6.0 does have the intrinsics, just not the method, another possibility would be to define something similar to CreateElementUnorderedAtomicMemMove (maybe even basing it off the implementation in LLVM 7+). This was just a placeholder until I got to making a better solution.

Nah I think it's fine to return nullptr here as these are unstable and we don't provide a hard guarantee about support for older LLVM (only a guarantee of support for some LLVM which we ship). The easiest thing to do would probably be to return null here and it can be expanded later if need be

Centril · 2019-03-13T17:34:13Z

r? @alexcrichton

alexcrichton · 2019-03-13T18:09:23Z

src/libcore/intrinsics.rs

@@ -962,6 +962,33 @@ extern "rust-intrinsic" {
    /// value is not necessarily valid to be used to actually access memory.
    pub fn arith_offset<T>(dst: *const T, offset: isize) -> *const T;

+    /// Equivalent to the appropriate `llvm.memcpy.p0i8.p0i8.*` intrinsic, with


Is this maybe a copy/paste typo? Should this mention that it's the atomic version?

yep, I'll fix that.

alexcrichton · 2019-03-13T18:09:55Z

Out of curiosity, could you expand a bit on the use cases for an atomic memcpy?

tmccombs · 2019-03-14T04:51:15Z

Out of curiosity, could you expand a bit on the use cases for an atomic memcpy?

To be honest, I don't know. This was just in "Call for Participation" in TWiR, and looked like something I could do.

@joshlf ?

joshlf · 2019-03-14T06:31:07Z

Sure thing! In Fuchsia, we often have two processes that communicate by sharing memory. If the two processes are mutually-distrusting, then, when operating on that shared memory, they each need to assume that the other is maliciously modifying the memory concurrently. From the perspective of the memory model, this is identical to another thread in the same process modifying the memory. Thus, to avoid UB, we can only operate on the memory using atomic operations. Our approach is very simple - copy all of the data into a separate part of the address space which is not shared, and then operate on it. Originally, we did this using a loop of Relaxed atomic loads, but that turned out to perform really poorly. Thus, we're hoping that these intrinsics - which have the guarantees that we need from a concurrency perspective - will perform better.

alexcrichton · 2019-03-14T14:18:09Z

Ok thanks for the explanation @joshlf! Makes sense to me.

@tmccombs mind also throwing in some codegen tests in src/test/codegen to exercise these code paths and test out various sizes and widths and such?

tmccombs · 2019-03-14T17:59:57Z

mind also throwing in some codegen tests in src/test/codegen to exercise these code paths and test out various sizes and widths and such?

sure thing

Centril · 2019-03-14T18:07:11Z

As long as we don't incur additional platform dependence or fundamental abilities when stably exposing this it seems fine to me from a T-Lang hat-perspective.

mtak- · 2019-03-16T17:57:21Z

In general this is useful for optimistic concurrency, where you read some memory, and then verify that the read was "OK" via some other mechanism (and typically retrying if it wasn't ok). I think this is currently the best tool for that under the C memory model.

Seqlocks could use it here: https://amanieu.github.io/seqlock/src/seqlock/src/lib.rs.html#147
STMs could use it: https://github.com/mtak-/swym/blob/1eeded6ea51bb7316ddf534ddb56105a7e24c011/src/internal/seq_storage.rs#L55

Some relevant discussion here: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0690r1.html

bors · 2019-03-30T01:02:55Z

☔ The latest upstream changes (presumably #58846) made this pull request unmergeable. Please resolve the merge conflicts.

WIP

tmccombs · 2019-04-05T04:56:08Z

Well, I'm a little stuck. Running the tests, I get this error:

/usr/bin/ld: /home/thayne/devel/rust-lang/build/x86_64-unknown-linux-gnu/test/run-pass/intrinsics/intrinsic-atomics/a.intrinsic_atomics.7rcbfp3g-cgu.2.rcgu.o: in function `intrinsic_atomics::main':
intrinsic_atomics.7rcbfp3g-cgu.2:(.text._ZN17intrinsic_atomics4main17h1af59ebac19920e7E+0x346): undefined reference to `__llvm_memcpy_element_unordered_atomic_4'
/usr/bin/ld: intrinsic_atomics.7rcbfp3g-cgu.2:(.text._ZN17intrinsic_atomics4main17h1af59ebac19920e7E+0x37d): undefined reference to `__llvm_memmove_element_unordered_atomic_4'
/usr/bin/ld: intrinsic_atomics.7rcbfp3g-cgu.2:(.text._ZN17intrinsic_atomics4main17h1af59ebac19920e7E+0x3b2): undefined reference to `__llvm_memset_element_unordered_atomic_4'
collect2: error: ld returned 1 exit status

I'm presuming that the references are generated by llvm, but I'm not sure what they are supposed to be linked to and why it isn't getting linked. Any help would be appreciated.

alexcrichton · 2019-04-05T15:14:55Z

LLVM will often lower intrinsic calls or other operations to function calls to various platform-specific intrinsics. Perhaps the best example are the memcpy/memmove intrinsics in LLVM, which may lower to literal calls to memcpy or memmove. In these situations LLVM expects that the host environment provides definitions for that functionality. More often than not the functionality is provided by compiler-builtins (the Rust version of compiler-rt), so this may be a case where that function is missing from compiler-builtins. Its presence should be confirmed in compiler-rt, though, and a reference implementation will likely also be found there.

If it's not there in compiler-rt, then unfortunately I'm not sure :(

bors · 2019-04-10T18:48:21Z

☔ The latest upstream changes (presumably #59810) made this pull request unmergeable. Please resolve the merge conflicts.

tmccombs · 2019-04-11T05:16:10Z

there does not appear to be an implementation in compiler-builtins :(

alexcrichton · 2019-04-11T14:56:32Z

If they're not present there this may unfortunately be a case where it's a bit too early to bind these intrinsics.

joshlf · 2019-04-12T02:16:49Z

Ah that's a shame. Is there anything we can do to unblock ourselves? Who controls compiler-builtins?

alexcrichton · 2019-04-12T15:07:13Z

I think to unblock this we need someone who knows how to implement, and I at least know that person is not me! The source of compiler-builtins is here, though.

Mark-Simulacrum · 2019-05-18T13:58:24Z

Visiting this for triage; I'm going to close this pull request as it looks like it's blocked on outstanding work in (probably) compiler builtins and is unlikely to make progress in the short term. Feel free to re-open once there's some progress, though!

joshlf · 2019-08-21T00:33:01Z

So folks on this thread are notified: rust-lang/compiler-builtins#311

joshlf · 2019-08-22T23:07:41Z

Now that rust-lang/compiler-builtins#311 has been merged, I think this can go forward?

tmccombs · 2019-09-16T07:56:17Z

I started working on this again, but ran into an issue where it complains that the new intrinsics are "unrecognized atomic operation"'s the first time libcore is compiled. I think when I first implemented this I fixed that by adding #[cfg(not(stage0))] but I wasn't sure if that was the right way to handle that, and even if it was, that doesn't seem to work anymore. Does anyone know how to deal with this?

tmccombs · 2019-09-17T07:55:24Z

Should I create a new pull request for this?

joshlf · 2019-10-07T19:23:09Z

@Ralith Might have thoughts on your issue.

m-ou-se · 2022-01-07T18:33:12Z

I was doing some GitHub archeology related to sequence locks and ended up here.

[..] #[cfg(not(stage0))] [..] doesn't seem to work anymore. Does anyone know how to deal with this?

Better late than never I suppose: The problem was that stage0 has been renamed to bootstrap.

Should I create a new pull request for this?

@tmccombs Did you end up opening a new PR? Or do you know if anything else happened towards adding these intrinsics after the last comments here?

Amanieu · 2022-01-07T20:25:49Z

LLVM's element-wise atomic operations actually generate very poor code in practice since they are always lowered to a call to the intrinsic. You can get much better codegen by using volatile reads and writes instead, which have similar (but stricter) guarantees.

tmccombs · 2022-01-08T00:21:57Z

@tmccombs Did you end up opening a new PR? Or do you know if anything else happened towards adding these intrinsics after the last comments here?

No, as far as I remember, I didn't do anything after my last comment.

rust-highfive assigned pnkfelix Mar 13, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2019

tmccombs force-pushed the element-atomic branch from 99c60bf to 63ed27a Compare March 13, 2019 07:58

tmccombs commented Mar 13, 2019

View reviewed changes

rust-highfive assigned alexcrichton and unassigned pnkfelix Mar 13, 2019

alexcrichton reviewed Mar 13, 2019

View reviewed changes

joshlf mentioned this pull request Mar 17, 2019

Use of volatile read is unsound Amanieu/seqlock#2

Closed

Add element-wise atomic memory operations

035fdfc

WIP

tmccombs force-pushed the element-atomic branch from ab89d83 to 035fdfc Compare March 31, 2019 06:18

Mark-Simulacrum added S-blocked-closed and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 18, 2019

Mark-Simulacrum closed this May 18, 2019

tarcieri mentioned this pull request May 19, 2019

Add LLVM atomic memcpy intrinsics, expose in core/std #58599

Open

Ralith mentioned this pull request Aug 20, 2019

Implement LLVM's elementwise unordered atomic memory intrinsics rust-lang/compiler-builtins#311

Merged

jyn514 added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-blocked-closed labels Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add element-wise atomic memory operations #59155

Add element-wise atomic memory operations #59155

tmccombs commented Mar 13, 2019

rust-highfive commented Mar 13, 2019

tmccombs Mar 13, 2019

tmccombs Mar 13, 2019

alexcrichton Mar 13, 2019

tmccombs Mar 14, 2019 •

edited

Loading

alexcrichton Mar 14, 2019

Centril commented Mar 13, 2019

alexcrichton Mar 13, 2019

tmccombs Mar 14, 2019

alexcrichton commented Mar 13, 2019

tmccombs commented Mar 14, 2019

joshlf commented Mar 14, 2019

alexcrichton commented Mar 14, 2019

tmccombs commented Mar 14, 2019

Centril commented Mar 14, 2019

mtak- commented Mar 16, 2019

bors commented Mar 30, 2019

tmccombs commented Apr 5, 2019

alexcrichton commented Apr 5, 2019

bors commented Apr 10, 2019

tmccombs commented Apr 11, 2019

alexcrichton commented Apr 11, 2019

joshlf commented Apr 12, 2019

alexcrichton commented Apr 12, 2019

Mark-Simulacrum commented May 18, 2019

joshlf commented Aug 21, 2019

joshlf commented Aug 22, 2019

tmccombs commented Sep 16, 2019

tmccombs commented Sep 17, 2019

joshlf commented Oct 7, 2019

m-ou-se commented Jan 7, 2022

Amanieu commented Jan 7, 2022

tmccombs commented Jan 8, 2022

Add element-wise atomic memory operations #59155

Add element-wise atomic memory operations #59155

Conversation

tmccombs commented Mar 13, 2019

rust-highfive commented Mar 13, 2019

tmccombs Mar 13, 2019

Choose a reason for hiding this comment

tmccombs Mar 13, 2019

Choose a reason for hiding this comment

alexcrichton Mar 13, 2019

Choose a reason for hiding this comment

tmccombs Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

alexcrichton Mar 14, 2019

Choose a reason for hiding this comment

Centril commented Mar 13, 2019

alexcrichton Mar 13, 2019

Choose a reason for hiding this comment

tmccombs Mar 14, 2019

Choose a reason for hiding this comment

alexcrichton commented Mar 13, 2019

tmccombs commented Mar 14, 2019

joshlf commented Mar 14, 2019

alexcrichton commented Mar 14, 2019

tmccombs commented Mar 14, 2019

Centril commented Mar 14, 2019

mtak- commented Mar 16, 2019

bors commented Mar 30, 2019

tmccombs commented Apr 5, 2019

alexcrichton commented Apr 5, 2019

bors commented Apr 10, 2019

tmccombs commented Apr 11, 2019

alexcrichton commented Apr 11, 2019

joshlf commented Apr 12, 2019

alexcrichton commented Apr 12, 2019

Mark-Simulacrum commented May 18, 2019

joshlf commented Aug 21, 2019

joshlf commented Aug 22, 2019

tmccombs commented Sep 16, 2019

tmccombs commented Sep 17, 2019

joshlf commented Oct 7, 2019

m-ou-se commented Jan 7, 2022

Amanieu commented Jan 7, 2022

tmccombs commented Jan 8, 2022

tmccombs Mar 14, 2019 •

edited

Loading