[PATCH] Abstract out calculation of max HWIs per wide int.

[PATCH] Abstract out calculation of max HWIs per wide int.

[Aldy Hernandez]

I'm about to add one more use of the same snippet of code, for a total
of 4 identical calculations in the code base.

This seems safe enough even before the release, since this file hardly
changes and I'm pretty much the only one who's touched it this year.

OK for trunk?

gcc/ChangeLog:

	* wide-int.h (WIDE_INT_MAX_HWIS): New.
	(class fixed_wide_int_storage): Use it.
	(trailing_wide_ints <N>::set_precision): Use it.
	(trailing_wide_ints <N>::extra_size): Use it.
---
 gcc/wide-int.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/gcc/wide-int.h b/gcc/wide-int.h
index a450a744c9f..6be343c0eb5 100644
--- a/gcc/wide-int.h
+++ b/gcc/wide-int.h
@@ -264,6 +264,10 @@ along with GCC; see the file COPYING3.  If not see
 /* The number of HWIs needed to store an offset_int.  */
 #define OFFSET_INT_ELTS (ADDR_MAX_PRECISION / HOST_BITS_PER_WIDE_INT)
 
+/* The max number of HWIs needed to store a wide_int of PRECISION.  */
+#define WIDE_INT_MAX_HWIS(PRECISION) \
+  ((PRECISION + HOST_BITS_PER_WIDE_INT - 1) / HOST_BITS_PER_WIDE_INT)
+
 /* The type of result produced by a binary operation on types T1 and T2.
    Defined purely for brevity.  */
 #define WI_BINARY_RESULT(T1, T2) \
@@ -1214,7 +1218,7 @@ template <int N>
 class GTY(()) fixed_wide_int_storage
 {
 private:
-  HOST_WIDE_INT val[(N + HOST_BITS_PER_WIDE_INT + 1) / HOST_BITS_PER_WIDE_INT];
+  HOST_WIDE_INT val[WIDE_INT_MAX_HWIS (N)];
   unsigned int len;
 
 public:
@@ -1475,8 +1479,7 @@ trailing_wide_ints <N>::set_precision (unsigned int precision,
   gcc_checking_assert (num_elements <= N);
   m_num_elements = num_elements;
   m_precision = precision;
-  m_max_len = ((precision + HOST_BITS_PER_WIDE_INT - 1)
-	       / HOST_BITS_PER_WIDE_INT);
+  m_max_len = WIDE_INT_MAX_HWIS (precision);
 }
 
 /* Return a reference to element INDEX.  */
@@ -1505,8 +1508,7 @@ inline size_t
 trailing_wide_ints <N>::extra_size (unsigned int precision,
 				    unsigned int num_elements)
 {
-  unsigned int max_len = ((precision + HOST_BITS_PER_WIDE_INT - 1)
-			  / HOST_BITS_PER_WIDE_INT);
+  unsigned int max_len = WIDE_INT_MAX_HWIS (precision);
   gcc_checking_assert (num_elements <= N);
   return (num_elements * max_len - 1) * sizeof (HOST_WIDE_INT);
 }
-- 
2.39.2

Re: [PATCH] Abstract out calculation of max HWIs per wide int.

[Andrew Pinski]

On Mon, Apr 17, 2023 at 11:44 AM Aldy Hernandez via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> I'm about to add one more use of the same snippet of code, for a total
> of 4 identical calculations in the code base.
>
> This seems safe enough even before the release, since this file hardly
> changes and I'm pretty much the only one who's touched it this year.
>
> OK for trunk?
>
> gcc/ChangeLog:
>
>         * wide-int.h (WIDE_INT_MAX_HWIS): New.
>         (class fixed_wide_int_storage): Use it.
>         (trailing_wide_ints <N>::set_precision): Use it.
>         (trailing_wide_ints <N>::extra_size): Use it.
> ---
>  gcc/wide-int.h | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/wide-int.h b/gcc/wide-int.h
> index a450a744c9f..6be343c0eb5 100644
> --- a/gcc/wide-int.h
> +++ b/gcc/wide-int.h
> @@ -264,6 +264,10 @@ along with GCC; see the file COPYING3.  If not see
>  /* The number of HWIs needed to store an offset_int.  */
>  #define OFFSET_INT_ELTS (ADDR_MAX_PRECISION / HOST_BITS_PER_WIDE_INT)
>
> +/* The max number of HWIs needed to store a wide_int of PRECISION.  */
> +#define WIDE_INT_MAX_HWIS(PRECISION) \
> +  ((PRECISION + HOST_BITS_PER_WIDE_INT - 1) / HOST_BITS_PER_WIDE_INT)

Does it make sense to use an constexpr inline function instead of a
define here since GCC is written in C++11 after all?
That is:
constexpr inline unsigned WIDE_INT_MAX_HWIS(unsigned precision)
{
  return ((precision + HOST_BITS_PER_WIDE_INT - 1) / HOST_BITS_PER_WIDE_INT);
}

Thanks,
Andrew Pinski

> +
>  /* The type of result produced by a binary operation on types T1 and T2.
>     Defined purely for brevity.  */
>  #define WI_BINARY_RESULT(T1, T2) \
> @@ -1214,7 +1218,7 @@ template <int N>
>  class GTY(()) fixed_wide_int_storage
>  {
>  private:
> -  HOST_WIDE_INT val[(N + HOST_BITS_PER_WIDE_INT + 1) / HOST_BITS_PER_WIDE_INT];
> +  HOST_WIDE_INT val[WIDE_INT_MAX_HWIS (N)];
>    unsigned int len;
>
>  public:
> @@ -1475,8 +1479,7 @@ trailing_wide_ints <N>::set_precision (unsigned int precision,
>    gcc_checking_assert (num_elements <= N);
>    m_num_elements = num_elements;
>    m_precision = precision;
> -  m_max_len = ((precision + HOST_BITS_PER_WIDE_INT - 1)
> -              / HOST_BITS_PER_WIDE_INT);
> +  m_max_len = WIDE_INT_MAX_HWIS (precision);
>  }
>
>  /* Return a reference to element INDEX.  */
> @@ -1505,8 +1508,7 @@ inline size_t
>  trailing_wide_ints <N>::extra_size (unsigned int precision,
>                                     unsigned int num_elements)
>  {
> -  unsigned int max_len = ((precision + HOST_BITS_PER_WIDE_INT - 1)
> -                         / HOST_BITS_PER_WIDE_INT);
> +  unsigned int max_len = WIDE_INT_MAX_HWIS (precision);
>    gcc_checking_assert (num_elements <= N);
>    return (num_elements * max_len - 1) * sizeof (HOST_WIDE_INT);
>  }
> --
> 2.39.2
>

Re: [PATCH] Abstract out calculation of max HWIs per wide int.

[Aldy Hernandez]

On 4/17/23 20:47, Andrew Pinski wrote:
> On Mon, Apr 17, 2023 at 11:44 AM Aldy Hernandez via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> I'm about to add one more use of the same snippet of code, for a total
>> of 4 identical calculations in the code base.
>>
>> This seems safe enough even before the release, since this file hardly
>> changes and I'm pretty much the only one who's touched it this year.
>>
>> OK for trunk?
>>
>> gcc/ChangeLog:
>>
>>          * wide-int.h (WIDE_INT_MAX_HWIS): New.
>>          (class fixed_wide_int_storage): Use it.
>>          (trailing_wide_ints <N>::set_precision): Use it.
>>          (trailing_wide_ints <N>::extra_size): Use it.
>> ---
>>   gcc/wide-int.h | 12 +++++++-----
>>   1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/gcc/wide-int.h b/gcc/wide-int.h
>> index a450a744c9f..6be343c0eb5 100644
>> --- a/gcc/wide-int.h
>> +++ b/gcc/wide-int.h
>> @@ -264,6 +264,10 @@ along with GCC; see the file COPYING3.  If not see
>>   /* The number of HWIs needed to store an offset_int.  */
>>   #define OFFSET_INT_ELTS (ADDR_MAX_PRECISION / HOST_BITS_PER_WIDE_INT)
>>
>> +/* The max number of HWIs needed to store a wide_int of PRECISION.  */
>> +#define WIDE_INT_MAX_HWIS(PRECISION) \
>> +  ((PRECISION + HOST_BITS_PER_WIDE_INT - 1) / HOST_BITS_PER_WIDE_INT)
> 
> Does it make sense to use an constexpr inline function instead of a
> define here since GCC is written in C++11 after all?
> That is:
> constexpr inline unsigned WIDE_INT_MAX_HWIS(unsigned precision)
> {
>    return ((precision + HOST_BITS_PER_WIDE_INT - 1) / HOST_BITS_PER_WIDE_INT);
> }

I am following the current style in wide-int.h, both in naming as well 
as macros, but I have no strong opinions.

I'm happy to do whatever y'all agree is best.
Aldy

Re: [PATCH] Abstract out calculation of max HWIs per wide int.

[Richard Biener]

On Mon, Apr 17, 2023 at 8:50 PM Aldy Hernandez via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 4/17/23 20:47, Andrew Pinski wrote:
> > On Mon, Apr 17, 2023 at 11:44 AM Aldy Hernandez via Gcc-patches
> > <gcc-patches@gcc.gnu.org> wrote:
> >>
> >> I'm about to add one more use of the same snippet of code, for a total
> >> of 4 identical calculations in the code base.
> >>
> >> This seems safe enough even before the release, since this file hardly
> >> changes and I'm pretty much the only one who's touched it this year.
> >>
> >> OK for trunk?
> >>
> >> gcc/ChangeLog:
> >>
> >>          * wide-int.h (WIDE_INT_MAX_HWIS): New.
> >>          (class fixed_wide_int_storage): Use it.
> >>          (trailing_wide_ints <N>::set_precision): Use it.
> >>          (trailing_wide_ints <N>::extra_size): Use it.
> >> ---
> >>   gcc/wide-int.h | 12 +++++++-----
> >>   1 file changed, 7 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/gcc/wide-int.h b/gcc/wide-int.h
> >> index a450a744c9f..6be343c0eb5 100644
> >> --- a/gcc/wide-int.h
> >> +++ b/gcc/wide-int.h
> >> @@ -264,6 +264,10 @@ along with GCC; see the file COPYING3.  If not see
> >>   /* The number of HWIs needed to store an offset_int.  */
> >>   #define OFFSET_INT_ELTS (ADDR_MAX_PRECISION / HOST_BITS_PER_WIDE_INT)
> >>
> >> +/* The max number of HWIs needed to store a wide_int of PRECISION.  */
> >> +#define WIDE_INT_MAX_HWIS(PRECISION) \
> >> +  ((PRECISION + HOST_BITS_PER_WIDE_INT - 1) / HOST_BITS_PER_WIDE_INT)
> >
> > Does it make sense to use an constexpr inline function instead of a
> > define here since GCC is written in C++11 after all?
> > That is:
> > constexpr inline unsigned WIDE_INT_MAX_HWIS(unsigned precision)
> > {
> >    return ((precision + HOST_BITS_PER_WIDE_INT - 1) / HOST_BITS_PER_WIDE_INT);
> > }

Hmm, but does that force inlining?  Not all touched contexts require a constant?

I'd be curious what C++ experts say here.

> I am following the current style in wide-int.h, both in naming as well
> as macros, but I have no strong opinions.

I'm OK with macros since as you say it follows existing style.

OK for trunk (but not the branch).

Richard.

> I'm happy to do whatever y'all agree is best.
> Aldy
>