From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-io1-xd2d.google.com (mail-io1-xd2d.google.com [IPv6:2607:f8b0:4864:20::d2d]) by sourceware.org (Postfix) with ESMTPS id F2CD9385040E for ; Fri, 25 Sep 2020 23:14:48 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F2CD9385040E Received: by mail-io1-xd2d.google.com with SMTP id g7so4846615iov.13 for ; Fri, 25 Sep 2020 16:14:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=wzLqERHWVqsUTV9T0YsywYIXucJsTZglRoMAzW60n2w=; b=dNynNWRrj43P3h+Y7JlLQsP8mi1QTXdX+rx5ulFb51D2JuAa5ZNXJGRmCEpfl0d9q4 0Eaffs2ViKZ4iloo4cE1yT3rshSJQhlMiRHQKMY7o3nDS2wM3TQ+DRR8qVqUAmmTQxgs a62DXZxU5TpB1iKvvjd7nAzCaoyiL20h5OwXvGDI/fN5AZQ8E6+sv9710MK6Le8V9w4/ RrwzTMUsnu7o/McmqLV/lVg+WPnaGoYd4oRr+zoPexXu/+jfFeYee8QfWjikdVxlDGq2 s7RRZBMYdf2dzQpXDKtJSl4e7O37cw65rdkB1h5zJpFk8tGOErwkYTYHqhBs/Vn5JYZk xADA== X-Gm-Message-State: AOAM530xRw0sqYS43mn4lpqQzsNeiKjNrvqwf8UboOdkkT347/GEQsez V97FVeBdmsjjem2exuZiboARiouuY6oHQRl9GiU= X-Google-Smtp-Source: ABdhPJxZqlLWidmTIuOl8uenc7LJnpw4fmJ4sCy5TXv3UY24wZ8HsT8NdU/iwFbybpoztcqXa0x7pVn87cJhqn0x/Fc= X-Received: by 2002:a02:486:: with SMTP id 128mr1167630jab.8.1601075688388; Fri, 25 Sep 2020 16:14:48 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Alistair Francis Date: Fri, 25 Sep 2020 16:03:29 -0700 Message-ID: Subject: Re: [PATCH v2 09/18] RISC-V: The ABI implementation for 32-bit To: Adhemerval Zanella Cc: GNU C Library Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Sep 2020 23:14:50 -0000 On Thu, Aug 27, 2020 at 12:43 PM Adhemerval Zanella via Libc-alpha wrote: > > > > On 10/08/2020 18:29, Alistair Francis via Libc-alpha wrote: > > On Fri, Jul 10, 2020 at 6:24 PM Maciej W. Rozycki wrote: > >> > >> On Fri, 10 Jul 2020, Alistair Francis wrote: > >> > >>>> I note there is an extensive discussion on the way to move forward here: > >>>> > >>>> We might as well try to implement it right away, so as to avoid being > >>>> limited to 32-bit time records here. > >>> > >>> Is there an advantage of doing it now or can we put this off for the > >>> next release? > >> > >> The change is major enough it'll have to wait for the next development > >> cycle anyway. It shouldn't matter that much for RV32 glibc deployments > >> though, given the amount of suitable hardware available. > >> > >>>> NB some existing ports do have __WORDSIZE_TIME64_COMPAT32 set and cleared > >>>> for their 64-bit and 32-bit ABIs respectively, as per the note in our > >>>> top-level bits/wordsize.h, however this reflects the state as before we > >>>> introduced the possibility for `__time_t' to be a 64-bit type with > >>>> `__WORDSIZE == 32' ABIs. Given the turn of events I think the note ought > >>>> to be updated accordingly; I gather it was missed with commit 07fe93cd9850 > >>>> ("generic/typesizes.h: Add support for 32-bit arches with 64-bit types"). > >> > >> Will you be able to look into it? > >> > >> The context here is before Y2038 changes __WORDSIZE_TIME64_COMPAT32 would > >> only be clear for 64-bit ABIs with those 64-bit systems that do not have > >> any 32-bit ABI (compatibility mode) to support, such as the DEC Alpha. > >> And it would always be clear for 32-bit ABIs, so as to use the proper > >> `__time_t' type without changing the width of actual data held there in > >> the structure. > >> > >> I'm not sure what the story is behind the S/390 port though; perhaps it > >> does not support ABI coexistence in a single run-time environment. > > > > I was just about to start this but Adhemerval has very kindly sent a > > patch series already. I'm going to leave this as is (defining it as 1 > > for RV32 and RV64). > > Don't forget to help on review ;). I'm trying :) > > > > > > >> > >>>>> int __cur_writer; > >>>>> int __shared; > >>>>> unsigned long int __pad1; > >>>>> unsigned long int __pad2; > >>>>> unsigned int __flags; > >>>>> +#else > >>>>> +# if __BYTE_ORDER == __BIG_ENDIAN > >>>>> + unsigned char __pad1; > >>>>> + unsigned char __pad2; > >>>>> + unsigned char __shared; > >>>>> + unsigned char __flags; > >>>>> +# else > >>>>> + unsigned char __flags; > >>>>> + unsigned char __shared; > >>>>> + unsigned char __pad1; > >>>>> + unsigned char __pad2; > >>>>> +# endif > >>>>> + int __cur_writer; > >>>>> +#endif > >>>>> }; > >>>> > >>>> I note with this change the RV32 structure will use the generic layout as > >>>> per sysdeps/nptl/bits/struct_rwlock.h, however regrettably RV64 does not. > >>>> Would it make sense to instead have the layout the same between RV64 and > >>>> RV32, perhaps by redefining `__pad1' and `__pad2' in terms of `unsigned > >>>> long long' (which would have alignment implications though) or otherwise? > >>> > >>> I'm not sure which one is better. On one hand it seems better to be > >>> more generic and therefore RV32 should use the generic interface. On > >>> the other hand the more similar they are the better. I'm still leaning > >>> towards we should be generic where possible. > >> > >> It would be good to get a second opinion here. > > > > The RV64 __pthread_rwlock_arch_t is exactly the same as the AArch64 > > version. My guess is that it was just copied to be the same. > > > > The generic version also has this comment > > > > /* Generic struct for both POSIX read-write lock. New ports are expected > > to use the default layout, however archictetures can redefine it to add > > arch-specific extensions (such as lock-elision). The struct have a size > > of 32 bytes on both LP32 and LP64 architectures. */ > > > > As we aren't adding any arch-specific extensions I think we should > > keep the generic one. It's unfortunate we didn't do that for RV64, but > > too late now. > > > > The only downside I see in using the generic version is what happens > > if the value of _shared or flags overflows a char. There are currently > > only 4 flags though, so I think it's ok. > > I come up with the generic interface by using the already one in place for > mostly 64-bit architecture and the internal layout is mainly to avoid duplicate > the internal type on multiple architectures (so we can use the generic > implementation). > > The only usage I can think of where same structure for RV64 and RV32 may > pose a role is to use a shared rwlock between a 32-bit and a 64-bit process > (pthread_rwlockattr_setpshared with PTHREAD_PROCESS_SHARED). But afaik this > is not supported on other architectures. Thanks for describing this. I'm going to stick with the current generic implementation for RV32 then. > > > > >> > >>>> Is there any benefit from having `__flags' and `__shared' (and the > >>>> reserve) grouped within a single 32-bit word? I gather there is, given > >>>> the lengths gone to to match the bit lanes across the word regardless of > >>>> the endianness. But what is it? > >>> > >>> I have no idea. > >> > >> Especially given this. > > The internal member that are most concurrently accessed by the common operations > (pthread_rwlock_{clock,timed}{wr,rd}lock) are the __readers, __writers, > __wrphase_futex, __writers_futex, and __cur_writer. The __shared and __flag > are either accessed with non-atomic operation or at initialization, so I don't > think there is much different in packing or not in a 32-bit word. My > understanding is architectures usually does that to avoid possible internal > padding, since the usual types for both member are uint8_t. Thanks for the info here as well. Alistair > > >> > >>>>> +# define _FP_DIV_MEAT_S(R, X, Y) _FP_DIV_MEAT_1_udiv_norm (S, R, X, Y) > >>>>> +# define _FP_DIV_MEAT_D(R, X, Y) _FP_DIV_MEAT_2_udiv (D, R, X, Y) > >>>>> +# define _FP_DIV_MEAT_Q(R, X, Y) _FP_DIV_MEAT_4_udiv (Q, R, X, Y) > >>>>> + > >>>>> +# define _FP_NANFRAC_S _FP_QNANBIT_S > >>>>> +# define _FP_NANFRAC_D _FP_QNANBIT_D, 0 > >>>>> +# define _FP_NANFRAC_Q _FP_QNANBIT_Q, 0, 0, 0 > >>>> > >>>> Likewise. There seems to be an established practice for this header > >>>> across architectures to have no space between macro arguments or before > >>>> the opening parenthesis. This might help with the alignment. > >>> > >>> I still think it makes sense to follow the glibc style though, even if > >>> other archs don't. > >>> > >>> Let me know if it should be a different way and I'll update it. > >> > >> There is the issue of the discrepancy compared to the libgcc version, and > >> while `diff -l' and `patch -l' solve that for manual processing, more > >> sophisticated tools may not cope and require manual intervention. > >> > >> Again, I would suggest getting a second opinion. > > > > The current option matches the rest of the file, so I think it makes > > sense to leave as is. > > > > If this is a problem we can just change the spacing for the entire > > file in a future patch. > > > > Alistair > > > >> > >> Maciej