diff mbox series

Change gcc/ira-conflicts.cc build_conflict_bit_table to use size_t/%zu

Message ID 4700e066-1b50-4e7b-92f7-d8c33a330bbf@gmail.com
State New
Headers show
Series Change gcc/ira-conflicts.cc build_conflict_bit_table to use size_t/%zu | expand

Commit Message

Jonathan Yong Feb. 1, 2024, 12:45 p.m. UTC 
Attached patch OK? Copied inline for review convenience.

Comments

Jakub Jelinek Feb. 1, 2024, 1:01 p.m. UTC | #1 
On Thu, Feb 01, 2024 at 12:45:31PM +0000, Jonathan Yong wrote:
> Attached patch OK? Copied inline for review convenience.

No, I think e.g. AIX doesn't support the z modifier.
I don't see %zd or %zu used anywhere except in gcc/jit/ which presumably
doesn't work on AIX.

If you really want to avoid truncation, perhaps do something like
  if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
    {
      if (sizeof (void *) <= sizeof (long))
	fprintf (ira_dump_file,
		 "+++Allocating %lu bytes for conflict table "
		 "(uncompressed size %lu)\n",
		 (unsigned long) (sizeof (IRA_INT_TYPE) * allocated_words_num),
		 (unsigned long) (sizeof (IRA_INT_TYPE) * object_set_words
				  * ira_objects_num));
      else
	fprintf (ira_dump_file,
		 "+++Allocating %l" PRIu64 "bytes for conflict table "
		 "(uncompressed size %" PRIu64 ")\n",
		 (unsigned HOST_WIDE_INT) (sizeof (IRA_INT_TYPE)
					   * allocated_words_num),
		 (unsigned HOST_WIDE_INT) (sizeof (IRA_INT_TYPE)
					   * object_set_words
					   * ira_objects_num));
    }

> diff --git a/gcc/ira-conflicts.cc b/gcc/ira-conflicts.cc
> index 671b4e42b6f..e966afe6cdc 100644
> --- a/gcc/ira-conflicts.cc
> +++ b/gcc/ira-conflicts.cc
> @@ -150,9 +150,9 @@ build_conflict_bit_table (void)
>    if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
>      fprintf
>        (ira_dump_file,
> -       "+++Allocating %ld bytes for conflict table (uncompressed size %ld)\n",
> -       (long) allocated_words_num * sizeof (IRA_INT_TYPE),
> -       (long) object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE));
> +       "+++Allocating %zu bytes for conflict table (uncompressed size %zu)\n",
> +       (size_t)(allocated_words_num * sizeof (IRA_INT_TYPE)),
> +       (size_t)(object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE)));
>    objects_live = sparseset_alloc (ira_objects_num);
>    for (i = 0; i < ira_max_point; i++)

> From 48861b8578526ac199b123ff71af8f9778c396c3 Mon Sep 17 00:00:00 2001
> From: Jonathan Yong <10walls@gmail.com>
> Date: Thu, 1 Feb 2024 12:35:52 +0000
> Subject: [PATCH] PR target/43613: use %zu for build_conflict_bit_table
> 
> LLP64 platforms like uses 32bit for long and may truncate. Use
> size_t and %zu to guarantee 64bit lengths.
> 
> gcc:
> 	*ira-conflicts.cc: use %zu for build_conflict_bit_table.
> ---
>  gcc/ira-conflicts.cc | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/ira-conflicts.cc b/gcc/ira-conflicts.cc
> index 671b4e42b6f..e966afe6cdc 100644
> --- a/gcc/ira-conflicts.cc
> +++ b/gcc/ira-conflicts.cc
> @@ -150,9 +150,9 @@ build_conflict_bit_table (void)
>    if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
>      fprintf
>        (ira_dump_file,
> -       "+++Allocating %ld bytes for conflict table (uncompressed size %ld)\n",
> -       (long) allocated_words_num * sizeof (IRA_INT_TYPE),
> -       (long) object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE));
> +       "+++Allocating %zu bytes for conflict table (uncompressed size %zu)\n",
> +       (size_t)(allocated_words_num * sizeof (IRA_INT_TYPE)),
> +       (size_t)(object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE)));
>  
>    objects_live = sparseset_alloc (ira_objects_num);
>    for (i = 0; i < ira_max_point; i++)
> -- 
> 2.43.0
> 


	Jakub
Xi Ruoyao Feb. 1, 2024, 1:06 p.m. UTC | #2 
On Thu, 2024-02-01 at 14:01 +0100, Jakub Jelinek wrote:
> On Thu, Feb 01, 2024 at 12:45:31PM +0000, Jonathan Yong wrote:
> > Attached patch OK? Copied inline for review convenience.
> 
> No, I think e.g. AIX doesn't support the z modifier.
> I don't see %zd or %zu used anywhere except in gcc/jit/ which presumably
> doesn't work on AIX.
> 
> If you really want to avoid truncation, perhaps do something like
>   if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
>     {
>       if (sizeof (void *) <= sizeof (long))
> 	fprintf (ira_dump_file,
> 		 "+++Allocating %lu bytes for conflict table "
> 		 "(uncompressed size %lu)\n",
> 		 (unsigned long) (sizeof (IRA_INT_TYPE) * allocated_words_num),
> 		 (unsigned long) (sizeof (IRA_INT_TYPE) * object_set_words
> 				  * ira_objects_num));
>       else
> 	fprintf (ira_dump_file,
> 		 "+++Allocating %l" PRIu64 "bytes for conflict table "
> 		 "(uncompressed size %" PRIu64 ")\n",

Should use HOST_WIDE_INT_PRINT_UNSIGNED instead of PRIu64.

> 		 (unsigned HOST_WIDE_INT) (sizeof (IRA_INT_TYPE)
> 					   * allocated_words_num),
> 		 (unsigned HOST_WIDE_INT) (sizeof (IRA_INT_TYPE)
> 					   * object_set_words
> 					   * ira_objects_num));
>     }
Jakub Jelinek Feb. 1, 2024, 1:13 p.m. UTC | #3 
On Thu, Feb 01, 2024 at 02:01:17PM +0100, Jakub Jelinek wrote:
> On Thu, Feb 01, 2024 at 12:45:31PM +0000, Jonathan Yong wrote:
> > Attached patch OK? Copied inline for review convenience.
> 
> No, I think e.g. AIX doesn't support the z modifier.
> I don't see %zd or %zu used anywhere except in gcc/jit/ which presumably
> doesn't work on AIX.

Or hwint.h could define PRIusize_t etc. macros and some corresponding type
to be used in casts, something like
#if SIZE_MAX == LONG_LONG_MAX
#define PRIusize_t HOST_LONG_LONG_FORMAT "u"
#define fmt_size_t unsigned long long
#elif SIZE_MAX == LONG_MAX
#define PRIusize_t HOST_LONG_FORMAT "u"
#define fmt_size_t unsigned long
#elif SIZE_MAX == INT_MAX
#define PRIusize_t "u"
#define fmt_size_t unsigned int
#else
#error "Unsupported size_t"
#endif

and then use
"... %" PRIusize_t " ... ", ... (fmt_size_t) (some_size_t_expr), ...

Or at configure time figure out which exact type size_t is say (unsigned
long long vs. unsigned long vs. unsigned int) using templates and
use that info to pick the right format modifier depending on that, then
the casts aren't needed.

	Jakub
Jonathan Yong Feb. 1, 2024, 1:42 p.m. UTC | #4 
On 2/1/24 13:06, Xi Ruoyao wrote:
> On Thu, 2024-02-01 at 14:01 +0100, Jakub Jelinek wrote:
>> On Thu, Feb 01, 2024 at 12:45:31PM +0000, Jonathan Yong wrote:
>>> Attached patch OK? Copied inline for review convenience.
>>
>> No, I think e.g. AIX doesn't support the z modifier.
>> I don't see %zd or %zu used anywhere except in gcc/jit/ which presumably
>> doesn't work on AIX.
>>
> 
> Should use HOST_WIDE_INT_PRINT_UNSIGNED instead of PRIu64.
> 
Updated the patch with the suggestions.

diff --git a/gcc/ira-conflicts.cc b/gcc/ira-conflicts.cc
index 671b4e42b6f..32688f9dc39 100644
--- a/gcc/ira-conflicts.cc
+++ b/gcc/ira-conflicts.cc
@@ -150,9 +150,9 @@ build_conflict_bit_table (void)
    if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
      fprintf
        (ira_dump_file,
-       "+++Allocating %ld bytes for conflict table (uncompressed size %ld)\n",
-       (long) allocated_words_num * sizeof (IRA_INT_TYPE),
-       (long) object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE));
+       "+++Allocating "HOST_WIDE_INT_PRINT_UNSIGNED" bytes for conflict table (uncompressed size "HOST_WIDE_INT_PRINT_UNSIGNED")\n",
+       (size_t)(allocated_words_num * sizeof (IRA_INT_TYPE)),
+       (size_t)(object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE)));
  
    objects_live = sparseset_alloc (ira_objects_num);
    for (i = 0; i < ira_max_point; i++)
Jakub Jelinek Feb. 1, 2024, 1:55 p.m. UTC | #5 
On Thu, Feb 01, 2024 at 01:42:03PM +0000, Jonathan Yong wrote:
> On 2/1/24 13:06, Xi Ruoyao wrote:
> > On Thu, 2024-02-01 at 14:01 +0100, Jakub Jelinek wrote:
> > > On Thu, Feb 01, 2024 at 12:45:31PM +0000, Jonathan Yong wrote:
> > > > Attached patch OK? Copied inline for review convenience.
> > > 
> > > No, I think e.g. AIX doesn't support the z modifier.
> > > I don't see %zd or %zu used anywhere except in gcc/jit/ which presumably
> > > doesn't work on AIX.
> > > 
> > 
> > Should use HOST_WIDE_INT_PRINT_UNSIGNED instead of PRIu64.
> > 
> Updated the patch with the suggestions.

No, that is wrong.  That will break bootstrap on lots of hosts, any time
size_t is not unsigned long (if unsigned long is 64-bit) or unsigned long
long (if unsigned long is not 64-bit).
That includes e.g. all targets where size_t is unsigned int, and some others
too.

	Jakub
Xi Ruoyao Feb. 1, 2024, 2:33 p.m. UTC | #6 
On Thu, 2024-02-01 at 14:55 +0100, Jakub Jelinek wrote:
> On Thu, Feb 01, 2024 at 01:42:03PM +0000, Jonathan Yong wrote:
> > On 2/1/24 13:06, Xi Ruoyao wrote:
> > > On Thu, 2024-02-01 at 14:01 +0100, Jakub Jelinek wrote:
> > > > On Thu, Feb 01, 2024 at 12:45:31PM +0000, Jonathan Yong wrote:
> > > > > Attached patch OK? Copied inline for review convenience.
> > > > 
> > > > No, I think e.g. AIX doesn't support the z modifier.
> > > > I don't see %zd or %zu used anywhere except in gcc/jit/ which presumably
> > > > doesn't work on AIX.
> > > > 
> > > 
> > > Should use HOST_WIDE_INT_PRINT_UNSIGNED instead of PRIu64.
> > > 
> > Updated the patch with the suggestions.

I mean if you are casting it to unsigned HOST_WIDE_INT, you should use
HOST_WIDE_INT_PRINT_UNSIGNED,  If you are casting it to size_t you
cannot use it (as Jakub has explained).

When you use printf-like things you have to keep the correspondence
between format specifier and the argument itself, 

> No, that is wrong.  That will break bootstrap on lots of hosts, any time
> size_t is not unsigned long (if unsigned long is 64-bit) or unsigned long
> long (if unsigned long is not 64-bit).
> That includes e.g. all targets where size_t is unsigned int, and some others
> too.
> 
> 	Jakub
>
Jakub Jelinek Feb. 1, 2024, 2:53 p.m. UTC | #7 
On Thu, Feb 01, 2024 at 02:13:11PM +0100, Jakub Jelinek wrote:
> Or hwint.h could define PRIusize_t etc. macros and some corresponding type
> to be used in casts, something like
> #if SIZE_MAX == LONG_LONG_MAX
> #define PRIusize_t HOST_LONG_LONG_FORMAT "u"
> #define fmt_size_t unsigned long long
> #elif SIZE_MAX == LONG_MAX
> #define PRIusize_t HOST_LONG_FORMAT "u"
> #define fmt_size_t unsigned long
> #elif SIZE_MAX == INT_MAX
> #define PRIusize_t "u"
> #define fmt_size_t unsigned int
> #else
> #error "Unsupported size_t"
> #endif
> 
> and then use
> "... %" PRIusize_t " ... ", ... (fmt_size_t) (some_size_t_expr), ...
> 
> Or at configure time figure out which exact type size_t is say (unsigned
> long long vs. unsigned long vs. unsigned int) using templates and
> use that info to pick the right format modifier depending on that, then
> the casts aren't needed.

Note, the former is probably better and might need some fallback, because
on some platforms size_t is some weird type like __uint20 or similar.
So maybe just start with smallest to largest, #if SIZE_MAX <= INT_MAX ...
#elif SIZE_MAX <= LONG_MAX ... #else ... #endif.

	Jakub
Jonathan Yong Feb. 1, 2024, 2:53 p.m. UTC | #8 
On 2/1/24 14:33, Xi Ruoyao wrote:
> 
> I mean if you are casting it to unsigned HOST_WIDE_INT, you should use
> HOST_WIDE_INT_PRINT_UNSIGNED,  If you are casting it to size_t you
> cannot use it (as Jakub has explained).
> 
> When you use printf-like things you have to keep the correspondence
> between format specifier and the argument itself,
> 
>> No, that is wrong.  That will break bootstrap on lots of hosts, any time
>> size_t is not unsigned long (if unsigned long is 64-bit) or unsigned long
>> long (if unsigned long is not 64-bit).
>> That includes e.g. all targets where size_t is unsigned int, and some others
>> too.
>>
>> 	Jakub
>>
> 

Thanks, that makes sense, tested on x86_64-linux.

diff --git a/gcc/ira-conflicts.cc b/gcc/ira-conflicts.cc
index 671b4e42b6f..ff5db18ffcc 100644
--- a/gcc/ira-conflicts.cc
+++ b/gcc/ira-conflicts.cc
@@ -150,9 +150,9 @@ build_conflict_bit_table (void)
    if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
      fprintf
        (ira_dump_file,
-       "+++Allocating %ld bytes for conflict table (uncompressed size %ld)\n",
-       (long) allocated_words_num * sizeof (IRA_INT_TYPE),
-       (long) object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE));
+       "+++Allocating "HOST_WIDE_INT_PRINT_UNSIGNED" bytes for conflict table (uncompressed size "HOST_WIDE_INT_PRINT_UNSIGNED")\n",
+       (unsigned HOST_WIDE_INT)(allocated_words_num * sizeof (IRA_INT_TYPE)),
+       (unsigned HOST_WIDE_INT)(object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE)));
  
    objects_live = sparseset_alloc (ira_objects_num);
    for (i = 0; i < ira_max_point; i++)
Jakub Jelinek Feb. 1, 2024, 2:55 p.m. UTC | #9 
On Thu, Feb 01, 2024 at 02:53:47PM +0000, Jonathan Yong wrote:
> On 2/1/24 14:33, Xi Ruoyao wrote:
> > 
> > I mean if you are casting it to unsigned HOST_WIDE_INT, you should use
> > HOST_WIDE_INT_PRINT_UNSIGNED,  If you are casting it to size_t you
> > cannot use it (as Jakub has explained).
> > 
> > When you use printf-like things you have to keep the correspondence
> > between format specifier and the argument itself,
> > 
> > > No, that is wrong.  That will break bootstrap on lots of hosts, any time
> > > size_t is not unsigned long (if unsigned long is 64-bit) or unsigned long
> > > long (if unsigned long is not 64-bit).
> > > That includes e.g. all targets where size_t is unsigned int, and some others
> > > too.
> > > 
> > > 	Jakub
> > > 
> > 
> 
> Thanks, that makes sense, tested on x86_64-linux.

No, besides the formatting being incorrect both in ChangeLog and in the
patch, this pessimizes ILP32 hosts unnecessarily.

> diff --git a/gcc/ira-conflicts.cc b/gcc/ira-conflicts.cc
> index 671b4e42b6f..ff5db18ffcc 100644
> --- a/gcc/ira-conflicts.cc
> +++ b/gcc/ira-conflicts.cc
> @@ -150,9 +150,9 @@ build_conflict_bit_table (void)
>    if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
>      fprintf
>        (ira_dump_file,
> -       "+++Allocating %ld bytes for conflict table (uncompressed size %ld)\n",
> -       (long) allocated_words_num * sizeof (IRA_INT_TYPE),
> -       (long) object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE));
> +       "+++Allocating "HOST_WIDE_INT_PRINT_UNSIGNED" bytes for conflict table (uncompressed size "HOST_WIDE_INT_PRINT_UNSIGNED")\n",
> +       (unsigned HOST_WIDE_INT)(allocated_words_num * sizeof (IRA_INT_TYPE)),
> +       (unsigned HOST_WIDE_INT)(object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE)));
>    objects_live = sparseset_alloc (ira_objects_num);
>    for (i = 0; i < ira_max_point; i++)

	Jakub
Jakub Jelinek Feb. 1, 2024, 3:25 p.m. UTC | #10 
On Thu, Feb 01, 2024 at 03:55:51PM +0100, Jakub Jelinek wrote:
> No, besides the formatting being incorrect both in ChangeLog and in the
> patch, this pessimizes ILP32 hosts unnecessarily.

So like this instead?

2024-02-01  Jakub Jelinek  <jakub@redhat.com>

	* hwint.h (GCC_PRISZ, fmt_size_t, HOST_SIZE_T_PRINT_DEC,
	HOST_SIZE_T_PRINT_UNSIGNED, HOST_SIZE_T_PRINT_HEX,
	HOST_SIZE_T_PRINT_HEX_PURE): Define.
	* ira-conflicts.cc (build_conflict_bit_table): Use it.  Formatting
	fixes.

--- gcc/hwint.h.jj	2024-01-03 11:51:32.676715299 +0100
+++ gcc/hwint.h	2024-02-01 16:22:53.037013522 +0100
@@ -115,6 +115,27 @@ typedef HOST_WIDE_INT __gcc_host_wide_in
 #define HOST_WIDE_INT_PRINT_DOUBLE_HEX "0x%" PRIx64 "%016" PRIx64
 #define HOST_WIDE_INT_PRINT_PADDED_HEX "%016" PRIx64
 
+/* Similarly format modifier for printing size_t.  As not all hosts support
+   z modifier in printf, use GCC_PRISZ and cast argument to fmt_size_t.
+   So, instead of doing fprintf ("%zu\n", sizeof (x) * y); use
+   fprintf (HOST_SIZE_T_PRINT_UNSIGNED "\n",
+	    (fmt_size_t) (sizeof (x) * y));  */
+#if SIZE_MAX <= INT_MAX
+# define GCC_PRISZ ""
+# define fmt_size_t unsigned int
+#elif SIZE_MAX <= LONG_MAX
+# define GCC_PRISZ HOST_LONG_FORMAT
+# define fmt_size_t unsigned long int
+#else
+# define GCC_PRISZ HOST_LONG_LONG_FORMAT
+# define fmt_size_t unsigned long long int
+#endif
+
+#define HOST_SIZE_T_PRINT_DEC "%" GCC_PRISZ "d"
+#define HOST_SIZE_T_PRINT_UNSIGNED "%" GCC_PRISZ "u"
+#define HOST_SIZE_T_PRINT_HEX "%#" GCC_PRISZ "x"
+#define HOST_SIZE_T_PRINT_HEX_PURE "%" GCC_PRISZ "x"
+
 /* Define HOST_WIDEST_FAST_INT to the widest integer type supported
    efficiently in hardware.  (That is, the widest integer type that fits
    in a hardware register.)  Normally this is "long" but on some hosts it
--- gcc/ira-conflicts.cc.jj	2024-01-03 11:51:31.748728178 +0100
+++ gcc/ira-conflicts.cc	2024-02-01 16:12:02.156137005 +0100
@@ -115,10 +115,10 @@ build_conflict_bit_table (void)
 	    > (uint64_t) param_ira_max_conflict_table_size * 1024 * 1024)
 	  {
 	    if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
-	      fprintf
-		(ira_dump_file,
-		 "+++Conflict table will be too big(>%dMB) -- don't use it\n",
-		 param_ira_max_conflict_table_size);
+	      fprintf (ira_dump_file,
+		       "+++Conflict table will be too big(>%dMB) "
+		       "-- don't use it\n",
+		       param_ira_max_conflict_table_size);
 	    return false;
 	  }
       }
@@ -148,11 +148,13 @@ build_conflict_bit_table (void)
 
   object_set_words = (ira_objects_num + IRA_INT_BITS - 1) / IRA_INT_BITS;
   if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
-    fprintf
-      (ira_dump_file,
-       "+++Allocating %ld bytes for conflict table (uncompressed size %ld)\n",
-       (long) allocated_words_num * sizeof (IRA_INT_TYPE),
-       (long) object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE));
+    fprintf (ira_dump_file,
+	     "+++Allocating " HOST_SIZE_T_PRINT_UNSIGNED
+	     " bytes for conflict table (uncompressed size "
+	     HOST_SIZE_T_PRINT_UNSIGNED ")\n",
+	     (fmt_size_t) (sizeof (IRA_INT_TYPE) * allocated_words_num),
+	     (fmt_size_t) (sizeof (IRA_INT_TYPE) * object_set_words
+			   * ira_objects_num));
 
   objects_live = sparseset_alloc (ira_objects_num);
   for (i = 0; i < ira_max_point; i++)


	Jakub
Jonathan Yong Feb. 1, 2024, 3:33 p.m. UTC | #11 
On 2/1/24 15:25, Jakub Jelinek wrote:
> On Thu, Feb 01, 2024 at 03:55:51PM +0100, Jakub Jelinek wrote:
>> No, besides the formatting being incorrect both in ChangeLog and in the
>> patch, this pessimizes ILP32 hosts unnecessarily.
> 
> So like this instead?
> 
> 2024-02-01  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* hwint.h (GCC_PRISZ, fmt_size_t, HOST_SIZE_T_PRINT_DEC,
> 	HOST_SIZE_T_PRINT_UNSIGNED, HOST_SIZE_T_PRINT_HEX,
> 	HOST_SIZE_T_PRINT_HEX_PURE): Define.
> 	* ira-conflicts.cc (build_conflict_bit_table): Use it.  Formatting
> 	fixes.
> 
> --- gcc/hwint.h.jj	2024-01-03 11:51:32.676715299 +0100
> +++ gcc/hwint.h	2024-02-01 16:22:53.037013522 +0100
> @@ -115,6 +115,27 @@ typedef HOST_WIDE_INT __gcc_host_wide_in
>   #define HOST_WIDE_INT_PRINT_DOUBLE_HEX "0x%" PRIx64 "%016" PRIx64
>   #define HOST_WIDE_INT_PRINT_PADDED_HEX "%016" PRIx64
>   
> +/* Similarly format modifier for printing size_t.  As not all hosts support
> +   z modifier in printf, use GCC_PRISZ and cast argument to fmt_size_t.
> +   So, instead of doing fprintf ("%zu\n", sizeof (x) * y); use
> +   fprintf (HOST_SIZE_T_PRINT_UNSIGNED "\n",
> +	    (fmt_size_t) (sizeof (x) * y));  */
> +#if SIZE_MAX <= INT_MAX
> +# define GCC_PRISZ ""
> +# define fmt_size_t unsigned int
> +#elif SIZE_MAX <= LONG_MAX
> +# define GCC_PRISZ HOST_LONG_FORMAT
> +# define fmt_size_t unsigned long int
> +#else
> +# define GCC_PRISZ HOST_LONG_LONG_FORMAT
> +# define fmt_size_t unsigned long long int
> +#endif
> +
> +#define HOST_SIZE_T_PRINT_DEC "%" GCC_PRISZ "d"
> +#define HOST_SIZE_T_PRINT_UNSIGNED "%" GCC_PRISZ "u"
> +#define HOST_SIZE_T_PRINT_HEX "%#" GCC_PRISZ "x"
> +#define HOST_SIZE_T_PRINT_HEX_PURE "%" GCC_PRISZ "x"
> +

...

> -      (ira_dump_file,
> -       "+++Allocating %ld bytes for conflict table (uncompressed size %ld)\n",
> -       (long) allocated_words_num * sizeof (IRA_INT_TYPE),
> -       (long) object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE));
> +    fprintf (ira_dump_file,
> +	     "+++Allocating " HOST_SIZE_T_PRINT_UNSIGNED
> +	     " bytes for conflict table (uncompressed size "
> +	     HOST_SIZE_T_PRINT_UNSIGNED ")\n",
> +	     (fmt_size_t) (sizeof (IRA_INT_TYPE) * allocated_words_num),
> +	     (fmt_size_t) (sizeof (IRA_INT_TYPE) * object_set_words
> +			   * ira_objects_num));
>   
>     objects_live = sparseset_alloc (ira_objects_num);
>     for (i = 0; i < ira_max_point; i++)
> 
> 
> 	Jakub
> 

Looks good for ILP32/LLP64.
Jonathan Yong Feb. 2, 2024, 11:43 p.m. UTC | #12 
On 2/1/24 15:33, Jonathan Yong wrote:
> On 2/1/24 15:25, Jakub Jelinek wrote:
>> On Thu, Feb 01, 2024 at 03:55:51PM +0100, Jakub Jelinek wrote:
>>> No, besides the formatting being incorrect both in ChangeLog and in the
>>> patch, this pessimizes ILP32 hosts unnecessarily.
>>
>> So like this instead?
>>
>> 2024-02-01  Jakub Jelinek  <jakub@redhat.com>
>>
>>     * hwint.h (GCC_PRISZ, fmt_size_t, HOST_SIZE_T_PRINT_DEC,
>>     HOST_SIZE_T_PRINT_UNSIGNED, HOST_SIZE_T_PRINT_HEX,
>>     HOST_SIZE_T_PRINT_HEX_PURE): Define.
>>     * ira-conflicts.cc (build_conflict_bit_table): Use it.  Formatting
>>     fixes.
>>
> 
> Looks good for ILP32/LLP64.
> 

Are you planning to push yet?
Jakub Jelinek Feb. 3, 2024, 12:03 a.m. UTC | #13 
On Fri, Feb 02, 2024 at 11:43:21PM +0000, Jonathan Yong wrote:
> On 2/1/24 15:33, Jonathan Yong wrote:
> > On 2/1/24 15:25, Jakub Jelinek wrote:
> > > On Thu, Feb 01, 2024 at 03:55:51PM +0100, Jakub Jelinek wrote:
> > > > No, besides the formatting being incorrect both in ChangeLog and in the
> > > > patch, this pessimizes ILP32 hosts unnecessarily.
> > > 
> > > So like this instead?
> > > 
> > > 2024-02-01  Jakub Jelinek  <jakub@redhat.com>
> > > 
> > >     * hwint.h (GCC_PRISZ, fmt_size_t, HOST_SIZE_T_PRINT_DEC,
> > >     HOST_SIZE_T_PRINT_UNSIGNED, HOST_SIZE_T_PRINT_HEX,
> > >     HOST_SIZE_T_PRINT_HEX_PURE): Define.
> > >     * ira-conflicts.cc (build_conflict_bit_table): Use it.  Formatting
> > >     fixes.
> > > 
> > 
> > Looks good for ILP32/LLP64.
> 
> Are you planning to push yet?

It needs to be reviewed first.

Passed successfully bootstrap/regtest on x86_64-linux and i686-linux.

Note, I think incrementally we should search for similar issues,
grep -C3 '%l[duxXi]' *.cc */*.cc | grep 'long)' | grep -v long.long | wc -l
101
is an upper bound on what should be looked at, I see at least a few dozens
from those (e.g. when the argument uses sizeof, or is say hash table
size () or elements () etc.).

Also, wonder if the pretty-printer.cc should get 'z' and 't' modifiers
added, for those we clearly can use 'z' or 't' when we implement it
ourselves.  But at least that part is stage1 material I'd say.
In translated messages we certainly can't use macros like
HOST_SIZE_T_PRINT_*, because it is then untranslatable.

	Jakub
Richard Biener Feb. 9, 2024, 10:33 a.m. UTC | #14 
On Thu, Feb 1, 2024 at 4:26 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Thu, Feb 01, 2024 at 03:55:51PM +0100, Jakub Jelinek wrote:
> > No, besides the formatting being incorrect both in ChangeLog and in the
> > patch, this pessimizes ILP32 hosts unnecessarily.
>
> So like this instead?

OK.

Thanks,
Richard.

> 2024-02-01  Jakub Jelinek  <jakub@redhat.com>
>
>         * hwint.h (GCC_PRISZ, fmt_size_t, HOST_SIZE_T_PRINT_DEC,
>         HOST_SIZE_T_PRINT_UNSIGNED, HOST_SIZE_T_PRINT_HEX,
>         HOST_SIZE_T_PRINT_HEX_PURE): Define.
>         * ira-conflicts.cc (build_conflict_bit_table): Use it.  Formatting
>         fixes.
>
> --- gcc/hwint.h.jj      2024-01-03 11:51:32.676715299 +0100
> +++ gcc/hwint.h 2024-02-01 16:22:53.037013522 +0100
> @@ -115,6 +115,27 @@ typedef HOST_WIDE_INT __gcc_host_wide_in
>  #define HOST_WIDE_INT_PRINT_DOUBLE_HEX "0x%" PRIx64 "%016" PRIx64
>  #define HOST_WIDE_INT_PRINT_PADDED_HEX "%016" PRIx64
>
> +/* Similarly format modifier for printing size_t.  As not all hosts support
> +   z modifier in printf, use GCC_PRISZ and cast argument to fmt_size_t.
> +   So, instead of doing fprintf ("%zu\n", sizeof (x) * y); use
> +   fprintf (HOST_SIZE_T_PRINT_UNSIGNED "\n",
> +           (fmt_size_t) (sizeof (x) * y));  */
> +#if SIZE_MAX <= INT_MAX
> +# define GCC_PRISZ ""
> +# define fmt_size_t unsigned int
> +#elif SIZE_MAX <= LONG_MAX
> +# define GCC_PRISZ HOST_LONG_FORMAT
> +# define fmt_size_t unsigned long int
> +#else
> +# define GCC_PRISZ HOST_LONG_LONG_FORMAT
> +# define fmt_size_t unsigned long long int
> +#endif
> +
> +#define HOST_SIZE_T_PRINT_DEC "%" GCC_PRISZ "d"
> +#define HOST_SIZE_T_PRINT_UNSIGNED "%" GCC_PRISZ "u"
> +#define HOST_SIZE_T_PRINT_HEX "%#" GCC_PRISZ "x"
> +#define HOST_SIZE_T_PRINT_HEX_PURE "%" GCC_PRISZ "x"
> +
>  /* Define HOST_WIDEST_FAST_INT to the widest integer type supported
>     efficiently in hardware.  (That is, the widest integer type that fits
>     in a hardware register.)  Normally this is "long" but on some hosts it
> --- gcc/ira-conflicts.cc.jj     2024-01-03 11:51:31.748728178 +0100
> +++ gcc/ira-conflicts.cc        2024-02-01 16:12:02.156137005 +0100
> @@ -115,10 +115,10 @@ build_conflict_bit_table (void)
>             > (uint64_t) param_ira_max_conflict_table_size * 1024 * 1024)
>           {
>             if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
> -             fprintf
> -               (ira_dump_file,
> -                "+++Conflict table will be too big(>%dMB) -- don't use it\n",
> -                param_ira_max_conflict_table_size);
> +             fprintf (ira_dump_file,
> +                      "+++Conflict table will be too big(>%dMB) "
> +                      "-- don't use it\n",
> +                      param_ira_max_conflict_table_size);
>             return false;
>           }
>        }
> @@ -148,11 +148,13 @@ build_conflict_bit_table (void)
>
>    object_set_words = (ira_objects_num + IRA_INT_BITS - 1) / IRA_INT_BITS;
>    if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
> -    fprintf
> -      (ira_dump_file,
> -       "+++Allocating %ld bytes for conflict table (uncompressed size %ld)\n",
> -       (long) allocated_words_num * sizeof (IRA_INT_TYPE),
> -       (long) object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE));
> +    fprintf (ira_dump_file,
> +            "+++Allocating " HOST_SIZE_T_PRINT_UNSIGNED
> +            " bytes for conflict table (uncompressed size "
> +            HOST_SIZE_T_PRINT_UNSIGNED ")\n",
> +            (fmt_size_t) (sizeof (IRA_INT_TYPE) * allocated_words_num),
> +            (fmt_size_t) (sizeof (IRA_INT_TYPE) * object_set_words
> +                          * ira_objects_num));
>
>    objects_live = sparseset_alloc (ira_objects_num);
>    for (i = 0; i < ira_max_point; i++)
>
>
>         Jakub
>
diff mbox series

Patch

From 48861b8578526ac199b123ff71af8f9778c396c3 Mon Sep 17 00:00:00 2001
From: Jonathan Yong <10walls@gmail.com>
Date: Thu, 1 Feb 2024 12:35:52 +0000
Subject: [PATCH] PR target/43613: use %zu for build_conflict_bit_table

LLP64 platforms like uses 32bit for long and may truncate. Use
size_t and %zu to guarantee 64bit lengths.

gcc:
	*ira-conflicts.cc: use %zu for build_conflict_bit_table.
---
 gcc/ira-conflicts.cc | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/gcc/ira-conflicts.cc b/gcc/ira-conflicts.cc
index 671b4e42b6f..e966afe6cdc 100644
--- a/gcc/ira-conflicts.cc
+++ b/gcc/ira-conflicts.cc
@@ -150,9 +150,9 @@  build_conflict_bit_table (void)
   if (internal_flag_ira_verbose > 0 && ira_dump_file != NULL)
     fprintf
       (ira_dump_file,
-       "+++Allocating %ld bytes for conflict table (uncompressed size %ld)\n",
-       (long) allocated_words_num * sizeof (IRA_INT_TYPE),
-       (long) object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE));
+       "+++Allocating %zu bytes for conflict table (uncompressed size %zu)\n",
+       (size_t)(allocated_words_num * sizeof (IRA_INT_TYPE)),
+       (size_t)(object_set_words * ira_objects_num * sizeof (IRA_INT_TYPE)));
 
   objects_live = sparseset_alloc (ira_objects_num);
   for (i = 0; i < ira_max_point; i++)
-- 
2.43.0