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

[Palmer Dabbelt <palmer@dabbelt.com>]

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);
    }
    
   
    $ 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