diff mbox

eliminate calls to snprintf(0, 0, ...) with known return value (pr78476)

Message ID f5968814-2dac-79b5-b94a-e74449f9ef4a@gmail.com
State New
Headers show

Commit Message

Martin Sebor Nov. 23, 2016, 3:02 a.m. UTC 
Calls to bounded functions like snprintf with a zero-size buffer
are special requests to compute the size of output without actually
writing any.  For example:

   int n = snprintf(0, 0, "%08x", rand ());

is a request to compute the number of bytes that the function would
format if it were passed a buffer of non-zero size.  In the example
above since the return value is known to be exactly 8, not only can
the snprintf return value be folded into a constant but the whole
call to snprintf can be eliminated.

The attached patch enables this optimization under the
-fprintf-return-value option.  The patch depends on the one for bug
78461 (posted earlier today) during the testing of which I noticed
that this optimization was missing from the gimple-ssa-sprintf pass.

Thanks
Martin

Comments

Jeff Law Nov. 23, 2016, 8:05 p.m. UTC | #1 
On 11/22/2016 08:02 PM, Martin Sebor wrote:
> Calls to bounded functions like snprintf with a zero-size buffer
> are special requests to compute the size of output without actually
> writing any.  For example:
>
>   int n = snprintf(0, 0, "%08x", rand ());
>
> is a request to compute the number of bytes that the function would
> format if it were passed a buffer of non-zero size.  In the example
> above since the return value is known to be exactly 8, not only can
> the snprintf return value be folded into a constant but the whole
> call to snprintf can be eliminated.
>
> The attached patch enables this optimization under the
> -fprintf-return-value option.  The patch depends on the one for bug
> 78461 (posted earlier today) during the testing of which I noticed
> that this optimization was missing from the gimple-ssa-sprintf pass.
>
> Thanks
> Martin
>
> gcc-78476.diff
>
>
> PR tree-optimization/78476 - snprintf(0, 0, ...) with known arguments not optimized away
>
> gcc/testsuite/ChangeLog:
>
> 	PR tree-optimization/78476
> 	* gcc.dg/tree-ssa/builtin-sprintf-5.c: New test.
>
> gcc/ChangeLog:
>
> 	PR tree-optimization/78476
> 	* gimple-ssa-sprintf.c (struct pass_sprintf_length::call_info):
> 	Add a member.
> 	(handle_gimple_call): Adjust signature.
> 	(try_substitute_return_value): Remove calls to bounded functions
> 	with zero buffer size whose result is known.
> 	(pass_sprintf_length::execute): Adjust call to handle_gimple_call.
OK.

I was initially a little surprised you didn't just delete the call from 
the IL and emit the trivial constant assignment.  Then I realized you 
probably would need to update the vuse/vdefs that were on the original 
call.  Using update_call_from_tree seems to take care of that for you.

I didn't see a negative test -- ie one that you shouldn't transform. 
Could you extract the one from c#0 in the referenced bz and add it as a 
negative test.  You can do that as a separate patch which you should 
consider pre-approved.

Jeff
Markus Trippelsdorf Nov. 25, 2016, 8:16 a.m. UTC | #2 
On 2016.11.22 at 20:02 -0700, Martin Sebor wrote:
> 
> gcc/ChangeLog:
> 
> 	PR tree-optimization/78476
> 	* gimple-ssa-sprintf.c (struct pass_sprintf_length::call_info):
> 	Add a member.
> 	(handle_gimple_call): Adjust signature.
> 	(try_substitute_return_value): Remove calls to bounded functions
> 	with zero buffer size whose result is known.
> 	(pass_sprintf_length::execute): Adjust call to handle_gimple_call.

You left conflict markers in gcc/ChangeLog in your r242854 commit.
Martin Sebor Nov. 28, 2016, 4:38 p.m. UTC | #3 
> OK.
>
> I was initially a little surprised you didn't just delete the call from
> the IL and emit the trivial constant assignment.  Then I realized you
> probably would need to update the vuse/vdefs that were on the original
> call.  Using update_call_from_tree seems to take care of that for you.
>
> I didn't see a negative test -- ie one that you shouldn't transform.
> Could you extract the one from c#0 in the referenced bz and add it as a
> negative test.  You can do that as a separate patch which you should
> consider pre-approved.

Thanks, that was a great catch!  I thought I had this covered but as
it turned out, I hadn't.  What's more, in the process of developing
the new test I uncovered a couple of gaps in the pass.  I opened bug
78521 to track them and posted a patch with a fix and a new test to
exercise both the absence of the optimization and that the newly
uncovered bugs are fixed:

   https://gcc.gnu.org/ml/gcc-patches/2016-11/msg02730.html

Martin
diff mbox

Patch

PR tree-optimization/78476 - snprintf(0, 0, ...) with known arguments not optimized away

gcc/testsuite/ChangeLog:

	PR tree-optimization/78476
	* gcc.dg/tree-ssa/builtin-sprintf-5.c: New test.

gcc/ChangeLog:

	PR tree-optimization/78476
	* gimple-ssa-sprintf.c (struct pass_sprintf_length::call_info):
	Add a member.
	(handle_gimple_call): Adjust signature.
	(try_substitute_return_value): Remove calls to bounded functions
	with zero buffer size whose result is known.
	(pass_sprintf_length::execute): Adjust call to handle_gimple_call.


Index: gcc/gimple-ssa-sprintf.c
===================================================================
--- gcc/gimple-ssa-sprintf.c	(revision 242703)
+++ gcc/gimple-ssa-sprintf.c	(working copy)
@@ -62,6 +62,7 @@  along with GCC; see the file COPYING3.  If not see
 #include "tree-object-size.h"
 #include "params.h"
 #include "tree-cfg.h"
+#include "tree-ssa-propagate.h"
 #include "calls.h"
 #include "cfgloop.h"
 #include "intl.h"
@@ -122,7 +123,7 @@  class pass_sprintf_length : public gimple_opt_pass
       fold_return_value = param;
     }
 
-  void handle_gimple_call (gimple_stmt_iterator);
+  void handle_gimple_call (gimple_stmt_iterator*);
 
   struct call_info;
   void compute_format_length (const call_info &, format_result *);
@@ -712,6 +713,11 @@  struct pass_sprintf_length::call_info
   /* True for functions like snprintf that specify the size of
      the destination, false for others like sprintf that don't.  */
   bool bounded;
+
+  /* True for bounded functions like snprintf that specify a zero-size
+     buffer as a request to compute the size of output without actually
+     writing any.  */
+  bool nowrite;
 };
 
 /* Return the result of formatting the '%%' directive.  */
@@ -2484,7 +2490,7 @@  get_destination_size (tree dest)
    have its range set to the range of return values, if that is known.  */
 
 static void
-try_substitute_return_value (gimple_stmt_iterator gsi,
+try_substitute_return_value (gimple_stmt_iterator *gsi,
 			     const pass_sprintf_length::call_info &info,
 			     const format_result &res)
 {
@@ -2500,15 +2506,30 @@  static void
       && (info.bounded || res.number_chars <= info.objsize)
       && res.number_chars - 1 <= target_int_max ())
     {
-      /* Replace the left-hand side of the call with the constant
-	 result of the formatted function minus 1 for the terminating
-	 NUL which the functions' return value does not include.  */
-      gimple_call_set_lhs (info.callstmt, NULL_TREE);
       tree cst = build_int_cst (integer_type_node, res.number_chars - 1);
-      gimple *g = gimple_build_assign (lhs, cst);
-      gsi_insert_after (&gsi, g, GSI_NEW_STMT);
-      update_stmt (info.callstmt);
 
+      if (info.nowrite)
+	{
+	  /* Replace the call to the bounded function with a zero size
+	     (e.g., snprintf(0, 0, "%i", 123) with the constant result
+	     of the function minus 1 for the terminating NUL which
+	     the function's  return value does not include.  */
+	  if (!update_call_from_tree (gsi, cst))
+	    gimplify_and_update_call_from_tree (gsi, cst);
+	  gimple *callstmt = gsi_stmt (*gsi);
+	  update_stmt (callstmt);
+	}
+      else
+	{
+	  /* Replace the left-hand side of the call with the constant
+	     result of the formatted function minus 1 for the terminating
+	     NUL which the function's return value does not include.  */
+	  gimple_call_set_lhs (info.callstmt, NULL_TREE);
+	  gimple *g = gimple_build_assign (lhs, cst);
+	  gsi_insert_after (gsi, g, GSI_NEW_STMT);
+	  update_stmt (info.callstmt);
+	}
+
       if (dump_file)
 	{
 	  location_t callloc = gimple_location (info.callstmt);
@@ -2517,7 +2538,8 @@  static void
 	  print_generic_expr (dump_file, cst, dump_flags);
 	  fprintf (dump_file, " for ");
 	  print_generic_expr (dump_file, info.func, dump_flags);
-	  fprintf (dump_file, " return value (output %s).\n",
+	  fprintf (dump_file, " %s (output %s).\n",
+		   info.nowrite ? "call" : "return value",
 		   res.constant ? "constant" : "variable");
 	}
     }
@@ -2582,11 +2604,11 @@  static void
    functions and if so, handle it.  */
 
 void
-pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator gsi)
+pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi)
 {
   call_info info = call_info ();
 
-  info.callstmt = gsi_stmt (gsi);
+  info.callstmt = gsi_stmt (*gsi);
   if (!gimple_call_builtin_p (info.callstmt, BUILT_IN_NORMAL))
     return;
 
@@ -2739,6 +2761,7 @@  void
 	 without actually producing any.  Pretend the size is
 	 unlimited in this case.  */
       info.objsize = HOST_WIDE_INT_MAX;
+      info.nowrite = true;
     }
   else
     {
@@ -2799,7 +2822,7 @@  pass_sprintf_length::execute (function *fun)
 	  gimple *stmt = gsi_stmt (si);
 
 	  if (is_gimple_call (stmt))
-	    handle_gimple_call (si);
+	    handle_gimple_call (&si);
 	}
     }
 
Index: gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-5.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-5.c	(revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/builtin-sprintf-5.c	(working copy)
@@ -0,0 +1,118 @@ 
+/* PR middle-end/78476 - snprintf(0, 0, ...) with known arguments not
+   optimized away
+   { dg-compile }
+   { dg-options "-O2 -fdump-tree-optimized" }
+   { dg-require-effective-target int32plus } */
+
+#define CAT(s, n)   s ## n
+#define FAIL(line)  CAT (failure_on_line_, line)
+#define PASS(line)  CAT (success_on_line_, line)
+
+/* Emit a call to a function named failure_on_line_NNN when EXPR is false.  */
+#define ASSERT(value, expect)			\
+  do {						\
+    extern void FAIL (__LINE__)(int);		\
+    extern void PASS (__LINE__)(int);		\
+    if (value == expect)			\
+      PASS (__LINE__)(value);			\
+    else					\
+      FAIL (__LINE__)(value);			\
+  } while (0)
+
+#define T(expect, ...)					\
+  do {							\
+    int n = __builtin_snprintf (0, 0, __VA_ARGS__);	\
+    ASSERT (n, expect);					\
+  } while (0)
+
+int ival (int i) { return i; }
+
+void test_arg_int (int i, int n)
+{
+  T (1, "%i", ival (0));
+  T (1, "%i", ival (1));
+  T (2, "%i%i", ival (0), ival (1));
+  T (3, "%i%i%i", ival (0), ival (1), ival (9));
+  T (5, "%i %i %i", ival (0), ival (1), ival (9));
+
+  T (5, "%i %i %i", ival (0), ival (1), ival (9));
+
+  T (13, "%hhu.%hhu.%hhu.%hhu", ival (23), ival (78), ival (216), ival (135));
+
+  for (i = 0; i != 9; ++i)
+    T (1, "%i", i);
+
+  for (i = -n; i != n; ++i)
+    T (8, "%08x", i);
+}
+
+void test_arg_string (const char *s)
+{
+  T ( 0, "%-s", "");
+  T ( 1, "%%");
+  T ( 1, "%-s", "1");
+  T ( 2, "%-s", "12");
+  T ( 3, "%-s", "123");
+  T ( 5, "s=%s", "123");
+  T (10, "%%s=\"%s\"", "12345");
+
+  T ( 1, "%.*s", 1, "123");
+  T ( 2, "%.*s", 2, "123");
+  T ( 3, "%.*s", 3, "123");
+  T ( 3, "%.*s", 4, "123");
+
+  T ( 1, "%1.*s", 1, "123");
+  T ( 2, "%1.*s", 2, "123");
+  T ( 3, "%1.*s", 3, "123");
+  T ( 3, "%1.*s", 4, "123");
+  T ( 4, "%4.*s", 1, "123");
+  T ( 4, "%4.*s", 2, "123");
+  T ( 4, "%4.*s", 3, "123");
+  T ( 4, "%4.*s", 4, "123");
+  T ( 4, "%4.*s", 5, "123");
+
+  const char *a = "123";
+  const char *b = "456";
+
+  T ( 3, "%-s", s ? a : b);
+  T ( 0, "%.0s", s);
+  T ( 1, "%1.1s", s);
+  T ( 2, "%2.2s", s);
+  T ( 2, "%2.1s", s);
+}
+
+void test_arg_multiarg (int i, double d)
+{
+  T (16, "%i %f %s", 123, 3.14, "abc");
+  T (16, "%12i %s", i, "abc");
+  T (16, "%*i %s", 12, i, "abc");
+}
+
+#define TV(expect, fmt, va)				\
+  do {							\
+    int n = __builtin_vsnprintf (0, 0, fmt, va);	\
+    ASSERT (n, expect);					\
+  } while (0)
+
+void test_va_int (__builtin_va_list va)
+{
+  TV ( 2, "%02hhx", va);
+  TV ( 2, "%02.*hhx", va);
+  TV ( 4, "%04hx", va);
+  TV ( 4, "%04.*hx", va);
+}
+
+void test_va_multiarg (__builtin_va_list va)
+{
+  TV ( 8, "%8x", va);
+  TV ( 8, "% 8x", va);
+  TV ( 9, "%9x", va);
+  TV (11, "%11o", va);
+  TV (12, "%12o", va);
+
+  TV (16, "%12i %3.2s", va);
+}
+
+
+/* { dg-final { scan-tree-dump-not "failure_on_line" "optimized"} }
+   { dg-final { scan-tree-dump-not "snprintf" "optimized"} } */