"Roger Sayle" <ro...@nextmovesoftware.com> 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 <typename T>
> +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;
> +}
> +
