From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 2820D3858433 for ; Tue, 30 May 2023 19:09:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 2820D3858433 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 29F9615DB; Tue, 30 May 2023 12:10:06 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5FE813F67D; Tue, 30 May 2023 12:09:20 -0700 (PDT) From: Richard Sandiford To: "Roger Sayle" Mail-Followup-To: "Roger Sayle" ,, richard.sandiford@arm.com Cc: Subject: Re: [PATCH] Refactor wi::bswap as a function (instead of a method). References: <005301d99195$e9524d60$bbf6e820$@nextmovesoftware.com> Date: Tue, 30 May 2023 20:09:19 +0100 In-Reply-To: <005301d99195$e9524d60$bbf6e820$@nextmovesoftware.com> (Roger Sayle's message of "Sun, 28 May 2023 19:55:00 +0100") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-28.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_SHORT,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: "Roger Sayle" writes: > This patch implements Richard Sandiford's suggestion from > https://gcc.gnu.org/pipermail/gcc-patches/2023-May/618215.html > that wi::bswap (and a new wi::bitreverse) should be functions, > and ideally only accessors are member functions. This patch > implements the first step, moving/refactoring wi::bswap. > > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap > and make -k check, both with and without --target_board=unix{-m32} > with no new failures. Ok for mainline? Thanks for doing this. OK with a minor change: > diff --git a/gcc/wide-int.h b/gcc/wide-int.h > index 3d9b87c..a2b3371 100644 > --- a/gcc/wide-int.h > +++ b/gcc/wide-int.h > @@ -552,6 +552,7 @@ namespace wi > UNARY_FUNCTION sext (const T &, unsigned int); > UNARY_FUNCTION zext (const T &, unsigned int); > UNARY_FUNCTION set_bit (const T &, unsigned int); > + UNARY_FUNCTION bswap (const T &); > > BINARY_FUNCTION min (const T1 &, const T2 &, signop); > BINARY_FUNCTION smin (const T1 &, const T2 &); > @@ -1086,9 +1087,6 @@ public: > static wide_int from_array (const HOST_WIDE_INT *, unsigned int, > unsigned int, bool = true); > static wide_int create (unsigned int); > - > - /* FIXME: target-dependent, so should disappear. */ > - wide_int bswap () const; > }; I think the comment was referring to the fact that the function swaps octets regardless of BITS_PER_UNIT. That might be the right behaviour, but it might not. It's difficult to say when there are no BITS_PER_UNIT!=8 targets left. The patch is moving in the right direction by defining the function outside the class. If it turns out that BITS_PER_UNIT should matter, we could later move the implementation to somewhere that is sensitive to the target. So... > @@ -2267,6 +2266,18 @@ wi::set_bit (const T &x, unsigned int bit) > return result; > } > > +/* Byte swap the integer X. */ ...how about adding: /* ??? This always swaps octets, regardless of BITS_PER_UNIT. If the function should instead be sensitive to BITS_PER_UNIT, we should either pass that in or move the function to somewhere where the target configuration is available. */ Feel free to reword to something you think is clearer. OK with that or a similar change, thanks. Richard > +template > +inline WI_UNARY_RESULT (T) > +wi::bswap (const T &x) > +{ > + WI_UNARY_RESULT_VAR (result, val, T, x); > + unsigned int precision = get_precision (result); > + WIDE_INT_REF_FOR (T) xi (x, precision); > + result.set_len (bswap_large (val, xi.val, xi.len, precision)); > + return result; > +} > +