Skip to content
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

Atomics #1059

Closed
bjorn3 opened this issue Oct 31, 2018 · 8 comments
Closed

Atomics #1059

bjorn3 opened this issue Oct 31, 2018 · 8 comments
Labels
cranelift:goal:rustc Focus area: Support for compiling Rust! cranelift:goal:spidermonkey Focus area: Support for using Cranelift in SpiderMonkey! cranelift Issues related to the Cranelift code generator

Comments

@bjorn3
Copy link
Contributor

bjorn3 commented Oct 31, 2018

I am currently emitting non atomic versions. This is doesnt have a high priority for me.

@lachlansneff
Copy link
Contributor

This is important for wasm as well.

@lachlansneff
Copy link
Contributor

As far as I can tell, this wouldn't be technically difficult to implement since atomic operations are the result of a LOCK prefix on some x86 instructions. I'm not sure about arm/riscv. The main issue here is the interface by which atomics would be exposed in codegen.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Nov 30, 2018

It would also be nice to implement pause on x86 / yield on arm. Those are by the way the only occurrences of inline asm in libcore. (https://github.com/rust-lang/rust/blob/cc63bd47efe4387982b399e2397adab6ac4030ef/src/libcore/sync/atomic.rs#L108-L118)

@lachlansneff
Copy link
Contributor

We could add a spin_loop_hint instruction.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Mar 29, 2019

Necessary instructions

  • atomic_fence
  • atomic_load
  • atomic_store
  • atomic_xchg
  • atomic_cxchg
  • atomic_xadd
  • atomic_xsub
  • atomic_and
  • atomic_nand
  • atomic_or
  • atomic_xor
  • atomic_max
  • atomic_umax
  • atomic_min
  • atomic_umin
  • spin_loop_hint

@bjorn3
Copy link
Contributor Author

bjorn3 commented Aug 20, 2019

@lars-t-hansen and I talked on irc (https://mozilla.logbot.info/cranelift/20190820#c16554457):

bjorn3: someone should implement atomics for cranelift
bjorn3: so threads should work with cg_clif
lth: bjorn3: noted.  i wonder how much work this is - not just raw implementation but ensuring that ordering guarantees are upheld through all transformations, etc
bjorn3: lth: using seqcst should just work I think when adding the necessary ops for for example atomic add
bjorn3: I don't think non MemFlags::READ_ONLY loads and stores are reordered at the moment
bjorn3: also on x86 aligned loads are already seqcst. the stores need the LOCK prefix: https://stackoverflow.com/questions/4972106/sequentially-consistent-atomic-load-on-x86/6495663#6495663
lth: yeah, i'm not worried about the fences, more about whether there are sufficient hooks in cranelift to prevent reordering in all cases or whether they have to be added too
lth: so, a little bit of an investigation at a minimum
bjorn3: lth: we could add a volatile MemFlag to prevent reordering, duplication and removal of mem ops
bjorn3: lth: that would be necessary to guarantee the rust volatile_{load,store} intrinsics to keep working correctly too
lth: are those volatile in the C++ sense or the Java sense?  (expect the latter, but don't know)
bjorn3: lth: whats the difference? I expect the former since they lower to llvm intrinsics, which is biased towards C/C++
lth: C++ volatile is Supremely Weird while Java volatile is basically a racy but otherwise indivisible access, iirc
lth: i see the docs point to C++
lth: (actually to C11)

@bjorn3
Copy link
Contributor Author

bjorn3 commented Aug 20, 2019

Proposal

Create a MemoryOrdering enum:

enum MemoryOrdering {
    NonAtomic,
    SeqCst,
    // Add more orderings if/when a weak memory model is implemented.
}

Modify the load and store instructions to accept an additional MemoryOrdering argument.

Add the following instructions:

  • spin_loop_hint
  • old = xchg ptr, new, memord
  • old = cxchg ptr, test_old, new, memord
  • old = mem_{iadd,isub} ptr, amount, memord
  • old = mem_{band,bnand,bor,bxor} ptr, val, memord
    • note: bnand is not the same as band_not
  • old = mem_{umax,smax,umin,smin} ptr, other, memord

When a weak memory model is implemented fence memord is necessary too.

@alexcrichton alexcrichton transferred this issue from bytecodealliance/cranelift Feb 28, 2020
@alexcrichton alexcrichton added cranelift:goal:rustc Focus area: Support for compiling Rust! cranelift:goal:spidermonkey Focus area: Support for using Cranelift in SpiderMonkey! cranelift Issues related to the Cranelift code generator labels Feb 28, 2020
@bjorn3
Copy link
Contributor Author

bjorn3 commented Aug 24, 2020

#2077 and #2149 implemented this for the AArch64 and x86_64 new-style backends respectively.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:goal:rustc Focus area: Support for compiling Rust! cranelift:goal:spidermonkey Focus area: Support for using Cranelift in SpiderMonkey! cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

No branches or pull requests

3 participants