diff mbox

[PR,middle-end/60832] Do not convert widest_int to tree just for printing it.

Message ID CAESRpQDkcbg6keczhaWNS7OdKf-QeE6SrA1yYMzijzUJLWg76w@mail.gmail.com
State New
Headers show

Commit Message

Manuel López-Ibáñez Sept. 20, 2015, 5:12 p.m. UTC 
In do_warn_aggressive_loop_optimizations, we convert to a tree just to print a
widest_int. Apart from overly complicated, this results in printing '3u'
instead of just '3'.

Unfortunately, adding a printf-like conversion specifier would require making
pretty-print.c link with wide-int.cc, which will include a lot of new
dependencies into several other programs (gcov-tool for example). It would be
possible to add the conversion specifier to every FE pretty-printer, but this
still would require updating c-format.c, which is far from trivial. A simpler
approach is to convert to a string rather than to a tree.

In addition, "iteration 3 invokes undefined behavior within this loop"
seems to me clearer than  "iteration 3 invokes undefined behavior;
containing loop".

Boot&tested in x86_64-linux-gnu

OK?

gcc/testsuite/ChangeLog:

2015-09-20  Manuel López-Ibáñez  <manu@gcc.gnu.org>

    PR middle-end/60832
    * gcc.dg/pr53265.c: Update.

gcc/ChangeLog:

2015-09-20  Manuel López-Ibáñez  <manu@gcc.gnu.org>

    PR middle-end/60832
    * tree-ssa-loop-niter.c (do_warn_aggressive_loop_optimizations):
    Print i_bound without converting it to a tree.

Comments

Richard Biener Sept. 21, 2015, 7:24 a.m. UTC | #1 
On Sun, 20 Sep 2015, Manuel López-Ibáñez wrote:

> In do_warn_aggressive_loop_optimizations, we convert to a tree just to print a
> widest_int. Apart from overly complicated, this results in printing '3u'
> instead of just '3'.
> 
> Unfortunately, adding a printf-like conversion specifier would require making
> pretty-print.c link with wide-int.cc, which will include a lot of new
> dependencies into several other programs (gcov-tool for example). It would be
> possible to add the conversion specifier to every FE pretty-printer, but this
> still would require updating c-format.c, which is far from trivial. A simpler
> approach is to convert to a string rather than to a tree.

I'd add the pretty printer overloads to wide-int-print.[ch], but yes
it would require a change in c-format.c.

> In addition, "iteration 3 invokes undefined behavior within this loop"
> seems to me clearer than  "iteration 3 invokes undefined behavior;
> containing loop".
> 
> Boot&tested in x86_64-linux-gnu
> 
> OK?

Ok.  Let's remember this case and if more of these pop up try working
on a "proper" solution.

Thanks,
Richard.

> gcc/testsuite/ChangeLog:
> 
> 2015-09-20  Manuel López-Ibáñez  <manu@gcc.gnu.org>
> 
>     PR middle-end/60832
>     * gcc.dg/pr53265.c: Update.
> 
> gcc/ChangeLog:
> 
> 2015-09-20  Manuel López-Ibáñez  <manu@gcc.gnu.org>
> 
>     PR middle-end/60832
>     * tree-ssa-loop-niter.c (do_warn_aggressive_loop_optimizations):
>     Print i_bound without converting it to a tree.
>
Manuel López-Ibáñez Sept. 21, 2015, 11:06 a.m. UTC | #2 
On 21 September 2015 at 09:24, Richard Biener <rguenther@suse.de> wrote:
> On Sun, 20 Sep 2015, Manuel López-Ibáñez wrote:
>
>> In do_warn_aggressive_loop_optimizations, we convert to a tree just to print a
>> widest_int. Apart from overly complicated, this results in printing '3u'
>> instead of just '3'.
>>
>> Unfortunately, adding a printf-like conversion specifier would require making
>> pretty-print.c link with wide-int.cc, which will include a lot of new
>> dependencies into several other programs (gcov-tool for example). It would be
>> possible to add the conversion specifier to every FE pretty-printer, but this
>> still would require updating c-format.c, which is far from trivial. A simpler
>> approach is to convert to a string rather than to a tree.
>
> I'd add the pretty printer overloads to wide-int-print.[ch], but yes
> it would require a change in c-format.c.

That is not enough, some conversion specifier parser needs to handle
the new codes. The one in pretty-print.c is used all over the place
and adding a dependency on wide-int.cc will bring gmp and other stuff
that is currently  not used (nor linked with) various helper programs.
If wide-int is only printed in the middle-end, perhaps it is
sufficient to add it to default_tree_printer. Otherwise, it needs to
be added to every FE. This is not difficult just tedious. The
difficult part is the c-format.c changes (I wish someone would fix
https://gcc.gnu.org/PR47781 so we can use a simpler syntax to define
our custom specifiers; in addition to the positive synergies it will
bring with other GNU projects that use custom specifiers and wish to
use -Wformat).

>> OK?
>
> Ok.  Let's remember this case and if more of these pop up try working
> on a "proper" solution.

Sure, I added my WIP patch to the PR, but I don't plan to keep working on it.

Cheers,

Manuel.
diff mbox

Patch

Index: gcc/tree-ssa-loop-niter.c
===================================================================
--- gcc/tree-ssa-loop-niter.c	(revision 227878)
+++ gcc/tree-ssa-loop-niter.c	(working copy)
@@ -2909,15 +2909,16 @@  do_warn_aggressive_loop_optimizations (s
   edge e = single_exit (loop);
   if (e == NULL)
     return;
 
   gimple estmt = last_stmt (e->src);
+  char buf[WIDE_INT_PRINT_BUFFER_SIZE];
+  print_dec (i_bound, buf, TYPE_UNSIGNED (TREE_TYPE (loop->nb_iterations))
+	     ? UNSIGNED : SIGNED);
   if (warning_at (gimple_location (stmt), OPT_Waggressive_loop_optimizations,
-		  "iteration %E invokes undefined behavior",
-		  wide_int_to_tree (TREE_TYPE (loop->nb_iterations),
-				    i_bound)))
-    inform (gimple_location (estmt), "containing loop");
+		  "iteration %s invokes undefined behavior", buf))
+    inform (gimple_location (estmt), "within this loop");
   loop->warned_aggressive_loop_optimizations = true;
 }
 
 /* Records that AT_STMT is executed at most BOUND + 1 times in LOOP.  IS_EXIT
    is true if the loop is exited immediately after STMT, and this exit
Index: gcc/testsuite/gcc.dg/pr53265.c
===================================================================
--- gcc/testsuite/gcc.dg/pr53265.c	(revision 227878)
+++ gcc/testsuite/gcc.dg/pr53265.c	(working copy)
@@ -10,36 +10,36 @@  void
 fn1 (void)
 {
   unsigned int a[128];
   int i;
 
-  for (i = 0; i < 128; ++i)	/* { dg-message "note: containing loop" } */
-    a[i] = i * 0x02000001;	/* { dg-warning "invokes undefined behavior" } */
+  for (i = 0; i < 128; ++i)	/* { dg-message "note: within this loop" } */
+    a[i] = i * 0x02000001;	/* { dg-warning "64 invokes undefined behavior" } */
   bar (a);
 }
 
 void
 fn2 (void)
 {
   unsigned long long a[128];
   int i;
 
-  for (i = 0; i < 128; i++)			/* { dg-message "note: containing loop" } */
-    a[i] = (i + 1LL) * 0x0123456789ABCDEFLL;	/* { dg-warning "invokes undefined behavior" } */
+  for (i = 0; i < 128; i++)			/* { dg-message "note: within this loop" } */
+    a[i] = (i + 1LL) * 0x0123456789ABCDEFLL;	/* { dg-warning "112 invokes undefined behavior" } */
   bar (a);
 }
 
 void
 fn3 (void)
 {
   unsigned char a[16], b[16], c[16];
   int i;
 
   bar (b);
-  for (i = 0; i < (int) (sizeof (a) / sizeof (a[0])); i++)	/* { dg-message "note: containing loop" } */
+  for (i = 0; i < (int) (sizeof (a) / sizeof (a[0])); i++)	/* { dg-message "note: within this loop" } */
     {
-      c[i + 8] = b[i];	/* { dg-warning "invokes undefined behavior" } */
+      c[i + 8] = b[i];	/* { dg-warning "8 invokes undefined behavior" } */
       a[i + 8] = b[i + 8];
     }
   bar (a);
   bar (c);
 }
@@ -48,13 +48,13 @@  void
 fn4 (void)
 {
   unsigned int *a[32], *o, i;
 
   bar (a);
-  for (i = 0; i <= sizeof (a) / sizeof (a[0]); i++)	/* { dg-message "note: containing loop" "" } */
+  for (i = 0; i <= sizeof (a) / sizeof (a[0]); i++)	/* { dg-message "note: within this loop" "" } */
     {
-      o = a[i];	/* { dg-warning "invokes undefined behavior" "" } */
+      o = a[i];	/* { dg-warning "32 invokes undefined behavior" "" } */
       bar (o);
     }
 }
 
 void
@@ -63,36 +63,36 @@  fn5 (void)
   unsigned short a[23940];
   unsigned int b[1140];
   int j;
 
   bar (b);
-  for (j = 0; j < 1140; j++)	/* { dg-message "note: containing loop" } */
-    a[23940 + j - 950] = b[j];	/* { dg-warning "invokes undefined behavior" } */
+  for (j = 0; j < 1140; j++)	/* { dg-message "note: within this loop" } */
+    a[23940 + j - 950] = b[j];	/* { dg-warning "950 invokes undefined behavior" } */
   bar (a);
 }
 
 void
 fn6 (void)
 {
   double a[4][3], b[12];
   int i;
   bar (b);
-  for (i = 0; i < 12; i++)	/* { dg-message "note: containing loop" } */
-    a[0][i] = b[i] / 10000.0;	/* { dg-warning "invokes undefined behavior" } */
+  for (i = 0; i < 12; i++)	/* { dg-message "note: within this loop" } */
+    a[0][i] = b[i] / 10000.0;	/* { dg-warning "3 invokes undefined behavior" } */
   bar (a);
 }
 
 void
 fn7 (void)
 {
   int a[16], b, c;
   bar (a);
-  for (b = a[c = 0]; c < 16; b = a[++c])	/* { dg-warning "invokes undefined behavior" "" } */
+  for (b = a[c = 0]; c < 16; b = a[++c])	/* { dg-warning "15 invokes undefined behavior" "" } */
     baz (b);
 }
 
-/* { dg-message "note: containing loop" "" { target *-*-* } 89 } */
+/* { dg-message "note: within this loop" "" { target *-*-* } 89 } */
 
 const void *va, *vb, *vc, *vd, *ve;
 const void *vf[4];
 void
 fn8 (void)
@@ -132,12 +132,12 @@  int xa[18];
 
 void
 fn10 (void)
 {
   int i;
-  for (i = 16; i < 32; i++)	/* { dg-message "note: containing loop" } */
-    xa[i] = 26;			/* { dg-warning "invokes undefined behavior" } */
+  for (i = 16; i < 32; i++)	/* { dg-message "note: within this loop" } */
+    xa[i] = 26;			/* { dg-warning "2 invokes undefined behavior" } */
 }
 
 __attribute__((noinline)) static void
 fn11 (int x)
 {