Re: [PATCH v2 00/10] [RISC-V] Atomics improvements [PR100265/PR100266]

[Palmer Dabbelt <palmer@dabbelt.com>]

On Fri, 14 Oct 2022 14:57:22 PDT (-0700), Palmer Dabbelt wrote:
> On Fri, 14 Oct 2022 13:39:33 PDT (-0700), jeffreyalaw@gmail.com wrote:
>>
>> On 10/14/22 05:03, Christoph Müllner wrote:
>>>
>>> My guess is people like the ISA mapping (more) because it has been 
>>> documented and reviewed.
>>> And it is the product of a working group that worked out the 
>>> RVWMO specification.
>>> This gives some confidence that we don't need to rework it massively 
>>> because of correctness issues in the future.
>>
>> This stuff can be hard and if someone with deep experience in memory 
>> models has reviewed the ISA mapping, then I'd prefer it over the GCC 
>> mapping.   It's just more likely the experts in the memory model space 
>> are more likely to get it right than a bunch of compiler junkies, no 
>> matter how smart we think we are :-)
>
> That's not really proven the case for the ISA suggested Linux mappings.  
> We've been through a bunch of rounds of review upstream and that's 
> resulted in some differences.  Some of that is likely related to the ISA 
> mappings for Linux being incomplete (they punt on the tricky bits like 
> interactions with spinlocks, which filter all over the place), and Linux 
> doesn't have the same old binary compatibility issues (aka the in-kernel 
> ABI is not stable between releases) so mapping churn isn't nearly as 
> scary over there.
>
> Still not much of an argument in favor of the GCC mappings, though.  I'm 
> pretty sure nobody with a formal memory model backgound has looked at 
> those, which is pretty much the worst spot to be in.  That said, I don't 
> think we can just say the ISA mappings are the way to go, they seem to 
> suffer from some similar incompleteness issues (for example, there's no 
> explicit mappings for std::atomic<T>::compare_exchange_{weak,strong}).  
> So we'll still need to put in the work to make sure whatever mappings 
> get implemented are correct.
>
> [
> As an aside, I think LLVM is doing the wrong thing for some of the more 
> complex forms of compare_exchange_weak.  For example
>
>     #include <atomic>
>     
>     bool f(std::atomic<long>& p, long& o, long n) {
>         return p.compare_exchange_weak(o, n, std::memory_order_acq_rel, std::memory_order_release);

Eric points out this is bogus code, the spec forbids release as the fail 
ordering (which akes sense).  Just kind of randomly permutating these 
ordering arguments ends up with some generated code that seems off, for 
example release/acq_rel produces lr/sc.rl (and no fences).

I don't think any of that is very relevant for the GCC discussion, 
though, as there's a bunch of other mappings specified by the ISA and we 
should just sort this one out.

>     }
>     
>    
>     $ clang-15.0.0 -std=c++17 -O3
>     f(std::atomic<long>&, long&, long):                   # @f(std::atomic<long>&, long&, long)
>             ld      a4, 0(a1)
>     .LBB0_3:                                # =>This Inner Loop Header: Depth=1
>             lr.d.aq a3, (a0)
>             bne     a3, a4, .LBB0_5
>             sc.d.rl a5, a2, (a0)
>             bnez    a5, .LBB0_3
>     .LBB0_5:
>             xor     a0, a3, a4
>             seqz    a0, a0
>             beq     a3, a4, .LBB0_2
>             sd      a3, 0(a1)
>     .LBB0_2:
>             ret
>
> doesn't look to me like it provides release ordering on failure, but I'm 
> not really a memory model person so maybe I'm missing something here.  
> The GCC mapping is pretty ugly, but I think we do happen to have correct 
> behavior in this case:
>
>     # gcc-12.2.0 -std=c++17 -O3
>     f(std::atomic<long>&, long&, long):
>             ld      a5,0(a1)
>             fence iorw,ow;  1: lr.d.aq a4,0(a0); bne a4,a5,1f; sc.d.aq a3,a2,0(a0); bnez a3,1b; 1:
>             sub     a5,a4,a5
>             seqz    a0,a5
>             beq     a5,zero,.L2
>             sd      a4,0(a1)
>     .L2:
>             andi    a0,a0,1
>             ret
> ]
>
>> Maybe I'm being too optimistic, but it's not hard for me to see a path 
>> where GCC and LLVM both implement the ISA mapping by default.  Anything 
>> else is just a path of long term pain.
>
> I'd bet that most people want that, but in practice building any real 
> systems in RISC-V land requires some degree of implementation-defined 
> behavior as the specifications don't cover everything (even ignoring the 
> whole PDF vs specification word games).  That's not to say we should 
> just ignore what's written down, just that there's more work to do even 
> if we ignore compatibility with old binaries.
>
> I think the question here is whether it's worth putting in the extra 
> work to provide a path for systems with old binaries to gradually 
> upgrade to the ISA mappings, or if we just toss out those old binaries.  
> I think we really need to see how bunch of a headache that compatibility 
> mode is going to be, and the only way to do that is put in the time to 
> analyze the GCC mappings.
>
> That said, I don't really personally care that much about old binaries.  
> Really my only argument here is that we broke binary compatibility once 
> (right before we upstreamed the port), that made a handful of people 
> mad, and I told them we'd never do it again.  I think we were all 
> operating under the assumption that RISC-V would move an order of 
> magnitude faster that it has, though, so maybe we're in a spot where 
> nobody actually cares about extant binaries and we can just throw them 
> all out.
>
> If we're going to do that, though, there's a bunch of other cruft that 
> we should probably toss along with the GCC mappings...
>
>>
>>
>> Jeff
>
> -- 
> You received this message because you are subscribed to the Google Groups "gnu-toolchain" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to gnu-toolchain+unsubscribe@rivosinc.com.
> To view this discussion on the web visit https://groups.google.com/a/rivosinc.com/d/msgid/gnu-toolchain/mhng-580bc331-c4d5-4f04-afd5-30f09f68b660%40palmer-ri-x1c9a.
> For more options, visit https://groups.google.com/a/rivosinc.com/d/optout.