rustc_target: [RISC-V] Enable non-CAS atomics on all targets

[Xiretza]

While targets without the atomic (A) ISA extension do not support CAS atomic operations, the standard guarantees that normal loads/stores to aligned addresses are atomic:

Furthermore, whereas naturally aligned loads and stores are guaranteed to execute atomically,
misaligned loads and stores might not, and hence require additional synchronization to ensure atomicity.

• RISC-V Unprivileged ISA V20191213, p. 25

This change is necessary for core::sync::atomics to contain at least .load() and .store() implementations for atomic types.

This was explicitly disabled in #66548 - @lenary, do we have a different interpretation of the standard, has the standard changed since that PR, or something else?

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @lcnr (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[jonas-schievink]

This matches what's done for the ARM/thumb targets, so this makes sense to me

[asb]

It's been a while since I dealt with the atomics lowering in the LLVM RISC-V backend, so I went back and tried to remind myself of the relevant points of this discussion. I think this is an incorrect change, unless something has changed since this was last investigated.

• The compiler-builtins doc still states "Rust only exposes atomic types on platforms that support them, and therefore does not need to fall back to software implementations.". I think this is still correct, and the consequence is that LLVM IR resulting in libcalls is undesirable?
• Checking what Thumb does, load atomic and store atomic do generate libcalls (see here). Do the core::sync::atomics.load()/store() not result in load atomic or store atomic IR?
• Due to the above, it's possible supporting atomic load/store on the thumb target is incorrect, as it presumably produces LLVM IR that will be lowered to libcalls

Is it correct for the LLVM backend to lower atomic load/store to libcalls? I believe so - when we implemented this, it was in line with other targets, reviewed by the RISC-V memory model working group (though it's possible they skimmed over the lowering for targets without the A extension), and follows LLVM's own guidance "on many older CPUs (e.g. ARMv5, SparcV8, Intel 80386) there are atomic load and store instructions, but no cmpxchg or LL/SC. As it is invalid to implement atomic load using the native instruction, but cmpxchg using a library call to a function that uses a mutex, atomic load must also expand to a library call on such architectures, so that it can remain atomic with regards to a simultaneous cmpxchg, by using the same mutex."

[Xiretza]

As it is invalid to implement atomic load using the native instruction, but cmpxchg using a library call to a function that uses a mutex

I have to admit I don't fully understand all the intricacies here, but to me this sounds like it should only be a problem if load/store use native instructions (simply l[whb]/s[whb] in RISC-V's case, since they are guaranteed to be atomic), but cmpxchg uses a software implementation. Shouldn't setting atomic_cas: false (as it always has been for these targets) disable the generation of those intrinsics? i.e. "Rust only exposes atomic types on platforms that support them, and therefore does not need to fall back to software implementations." still applies because only the load/store intrinsics, which are natively supported, are exposed.

[luismarques]

I have to admit I don't fully understand all the intricacies here, but to me this sounds like it should only be a problem if load/store use native instructions (simply l[whb]/s[whb] in RISC-V's case, since they are guaranteed to be atomic), but cmpxchg uses a software implementation.

You can still have a problem with just plain atomic loads and stores. Besides the "indivisibility" of the atomic, you also need to guarantee the appropriate memory orderings. With RISC-V atomics you have fences. With the base RVI ISA you need the libcall to provide that guarantee by some other means.

[Xiretza]

You can still have a problem with just plain atomic loads and stores. Besides the "indivisibility" of the atomic, you also need to guarantee the appropriate memory orderings. With RISC-V atomics you have fences. With the base RVI ISA you need the libcall to provide that guarantee by some other means.

Ah yes, that makes sense. Isn't FENCE in the base instruction set though and could just be generated? Or does that need a libcall for some reason?

[asb]

FENCE is in the base instruction set. The issue is that even if you disallow support for the RMW operations implied by atomic_cas: false, LLVM has no way to know you're doing that, and no way to know you're trying to describe a system where it's assumed those libcalls can't possibly be generated. Even with atomic_case: false, you'd have potential problems if e.g. there was C code on the system that was using those RMW libcalls.

Therefore, I think if you wanted to support "atomic" load/store with the assumption that none of the other libcalls are running on the system, you'd have to lower it yourself to something other than LLVM's atomic load and store operations.

[Xiretza]

Hm, that's unfortunate. The issue that prompted this PR is Amanieu/parking_lot#277, where the library assumes that core::sync::atomic::*.load()/store() are also available on platforms without RMW operations. So is this just a wrong assumption, or is there something that can be done on the rust side to make it true?

When trying to recreate this locally using a custom target JSON, linking fails because it can't find the symbols __atomic_(load/store)_1. Assuming this isn't caused by a broken environment, and if I understand correctly that these are the names of libcalls, one possible reason for this that I can think of is that LLVM creates either all atomic libcalls or none. This would then explain why it can't rely on the RMW libcalls not being used (something else could link against them). If my understanding here is correct, could a solution be to pass the atomic_cas target property to LLVM, which could then make sure no RMW libcalls are used?

Aside: what is atomic_cas: false supposed to be used for, if it's apparently not safe to have only non-CAS operations supported natively? Wouldn't this make it equivalent to a has_atomics property?

Therefore, I think if you wanted to support "atomic" load/store with the assumption that none of the other libcalls are running on the system, you'd have to lower it yourself to something other than LLVM's atomic load and store operations.

What exactly do you mean by "lowering it myself"? In user code, or would this be something in rustc?

[asb]

Hm, that's unfortunate. The issue that prompted this PR is Amanieu/parking_lot#277, where the library assumes that core::sync::atomic::*.load()/store() are also available on platforms without RMW operations. So is this just a wrong assumption, or is there something that can be done on the rust side to make it true?

That's what I meant by "lower it yourself" - where by "yourself", I meant rustc. There certainly are platforms that support atomic load/store but not RMW, but you're not able to use those atomic load/store operations directly if you're trying to support something like the C11 memory model, by my understanding (due to the problem described below when intermingling libcalls-based RMW operations with plain load/store).

When trying to recreate this locally using a custom target JSON, linking fails because it can't find the symbols __atomic_(load/store)_1. Assuming this isn't caused by a broken environment, and if I understand correctly that these are the names of libcalls, one possible reason for this that I can think of is that LLVM creates either all atomic libcalls or none. This would then explain why it can't rely on the RMW libcalls not being used (something else could link against them). If my understanding here is correct, could a solution be to pass the atomic_cas target property to LLVM, which could then make sure no RMW libcalls are used?

That's right - it's all libcalls or none, as mentioned in the LLVM Atomic Instructions and Concurrency Guide I linked to. Theoretically you could modify LLVM to support this notion of a system where atomic load/store should be lowered to native load/store and RMW operations aren't supported, but it's not something LLVM supports today.

Aside: what is atomic_cas: false supposed to be used for, if it's apparently not safe to have only non-CAS operations supported natively? Wouldn't this make it equivalent to a has_atomics property?

I wonder if it's a mistake. There's relevant discussion on this issue in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=86005 (which a colleague recently reminded me I'd submitted!) and James Y Knight has a nice example in the last comment demonstrating the sort of issues in intermingling libcall-based RMW operations and native load/stores.

[Xiretza]

Alright, closing this then - there's no simple straight-forward solution, maybe I'll get my hands dirty and fix it properly at some point, but that'll be a different PR. For now, simply building for an A-extension target and making sure not to use anything that would generate A-extension instructions is the best workaround for me.

[jonas-schievink]

• Checking what Thumb does, load atomic and store atomic do generate libcalls (see here).

I don't understand the full picture here, but since those are __sync_ libcalls they are fine to mix with native atomic instructions. Nevertheless, Rust's thumbv6m-none-eabi target does not emit any libcalls. This could be because ARMv6-M has a native data memory barrier instruction that is used by the lowering (the test you linked targets thumbv6-apple-ios, which is ARMv6-A).