Re: [PATCH] Optimize away useless snprintf calls with -fprintf-return-value (take 2)

[Jeff Law]

On 01/10/2017 12:23 AM, Jakub Jelinek wrote:

On Mon, Jan 02, 2017 at 04:54:57PM -0700, Martin Sebor wrote:

Looks good to me, thanks!  Just a couple of suggestions:


Here is updated patch with those added comments (and adjusted for the
current trunk).  Ok for trunk?

2017-01-10  Jakub Jelinek  

* gimple-ssa-sprintf.c (try_substitute_return_value): Remove
info.nowrite calls with no lhs that can't throw.  Return bool
whether gsi_remove has been called or not.
(pass_sprintf_length::handle_gimple_call): Return bool whether
try_substitute_return_value called gsi_remove.  Formatting fix.
(pass_sprintf_length::execute): Don't use gsi_remove if
handle_gimple_call returned true.

* gcc.dg/tree-ssa/builtin-snprintf-1.c: New test.

OK.
jeff

[PATCH] Optimize away useless snprintf calls with -fprintf-return-value (take 2)

[Jakub Jelinek]

On Mon, Jan 02, 2017 at 04:54:57PM -0700, Martin Sebor wrote:
> Looks good to me, thanks!  Just a couple of suggestions:

Here is updated patch with those added comments (and adjusted for the
current trunk).  Ok for trunk?

2017-01-10  Jakub Jelinek  

* gimple-ssa-sprintf.c (try_substitute_return_value): Remove
info.nowrite calls with no lhs that can't throw.  Return bool
whether gsi_remove has been called or not.
(pass_sprintf_length::handle_gimple_call): Return bool whether
try_substitute_return_value called gsi_remove.  Formatting fix.
(pass_sprintf_length::execute): Don't use gsi_remove if
handle_gimple_call returned true.

* gcc.dg/tree-ssa/builtin-snprintf-1.c: New test.

--- gcc/gimple-ssa-sprintf.c.jj 2017-01-09 11:35:03.769828764 +0100
+++ gcc/gimple-ssa-sprintf.c2017-01-10 08:21:34.185771116 +0100
@@ -128,7 +128,7 @@ public:
   fold_return_value = param;
 }
 
-  void handle_gimple_call (gimple_stmt_iterator*);
+  bool handle_gimple_call (gimple_stmt_iterator *);
 
   struct call_info;
   bool compute_format_length (call_info &, format_result *);
@@ -2735,9 +2735,11 @@ get_destination_size (tree dest)
described by INFO, substitute the result for the return value of
the call.  The result is suitable if the number of bytes it represents
is known and exact.  A result that isn't suitable for substitution may
-   have its range set to the range of return values, if that is known.  */
+   have its range set to the range of return values, if that is known.
+   Return true if the call is removed and gsi_next should not be performed
+   in the caller.  */
 
-static void
+static bool
 try_substitute_return_value (gimple_stmt_iterator *gsi,
 const pass_sprintf_length::call_info ,
 const format_result )
@@ -2797,6 +2799,24 @@ try_substitute_return_value (gimple_stmt
   res.constant ? "constant" : "variable");
}
 }
+  else if (lhs == NULL_TREE
+  && info.nowrite
+  && !stmt_ends_bb_p (info.callstmt))
+{
+  /* Remove the call to the bounded function with a zero size
+(e.g., snprintf(0, 0, "%i", 123)) if there is no lhs.  */
+  unlink_stmt_vdef (info.callstmt);
+  gsi_remove (gsi, true);
+  if (dump_file)
+   {
+ location_t callloc = gimple_location (info.callstmt);
+ fprintf (dump_file, "On line %i removing ",
+  LOCATION_LINE (callloc));
+ print_generic_expr (dump_file, info.func, dump_flags);
+ fprintf (dump_file, " call.\n");
+   }
+  return true;
+}
   else
 {
   unsigned HOST_WIDE_INT maxbytes;
@@ -2852,19 +2872,22 @@ try_substitute_return_value (gimple_stmt
 inbounds, (unsigned long)res.number_chars - 1, ign);
}
 }
+
+  return false;
 }
 
 /* Determine if a GIMPLE CALL is to one of the sprintf-like built-in
-   functions and if so, handle it.  */
+   functions and if so, handle it.  Return true if the call is removed
+   and gsi_next should not be performed in the caller.  */
 
-void
+bool
 pass_sprintf_length::handle_gimple_call (gimple_stmt_iterator *gsi)
 {
   call_info info = call_info ();
 
   info.callstmt = gsi_stmt (*gsi);
   if (!gimple_call_builtin_p (info.callstmt, BUILT_IN_NORMAL))
-return;
+return false;
 
   info.func = gimple_call_fndecl (info.callstmt);
   info.fncode = DECL_FUNCTION_CODE (info.func);
@@ -2955,7 +2978,7 @@ pass_sprintf_length::handle_gimple_call
   break;
 
 default:
-  return;
+  return false;
 }
 
   /* The first argument is a pointer to the destination.  */
@@ -3019,11 +3042,9 @@ pass_sprintf_length::handle_gimple_call
 }
 
   if (idx_objsize != HOST_WIDE_INT_M1U)
-{
-  if (tree size = gimple_call_arg (info.callstmt, idx_objsize))
- if (tree_fits_uhwi_p (size))
-   objsize = tree_to_uhwi (size);
-}
+if (tree size = gimple_call_arg (info.callstmt, idx_objsize))
+  if (tree_fits_uhwi_p (size))
+   objsize = tree_to_uhwi (size);
 
   if (info.bounded && !dstsize)
 {
@@ -3048,7 +3069,7 @@ pass_sprintf_length::handle_gimple_call
  location_t loc = gimple_location (info.callstmt);
  warning_at (EXPR_LOC_OR_LOC (dstptr, loc),
  info.warnopt (), "null destination pointer");
- return;
+ return false;
}
 
   /* Set the object size to the smaller of the two arguments
@@ -3077,12 +3098,12 @@ pass_sprintf_length::handle_gimple_call
   location_t loc = gimple_location (info.callstmt);
   warning_at (EXPR_LOC_OR_LOC (info.format, loc),
  info.warnopt (), "null format string");
-  return;
+  return false;
 }
 
   info.fmtstr = get_format_string (info.format, );
   if (!info.fmtstr)
-return;
+return false;
 
   /* The result is the number of bytes output by the formatted