[PATCH] Devirtualization dump functions fix

[PATCH] Devirtualization dump functions fix

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1133 bytes --]

Hello,
    I encountered similar issue to PR ipa/61462 where location_t locus = gimple_location (e->call_stmt) is called for e->call_stmt == NULL (Firefox with -flto -fdump-ipa-devirt). So that, I decided to introduce new function that is called for all potentially unsafe locations. I am wondering if a newly added function can be added in more seamless way (without playing with va_list and ATTRIBUTE_PRINTF stuff)?

Bootstrapped and regtested on x86_64-unknown-linux-gnu.

Thanks,
Martin

     ChangeLog:

     2014-06-26  Martin Liska  <mliska@suse.cz>

         * include/ansidecl.h: New collection of ATTRIBUTE_NULL_PRINTF_X_0
         defined.

     gcc/ChangeLog:

     2014-06-26  Martin Liska  <mliska@suse.cz>

         * dumpfile.h: New function dump_printf_loc_for_stmt.
         * dumpfile.c: Implementation added.
         (dump_vprintf): New function.i
         * cgraphunit.c: dump_printf_loc_for_stmt usage replaces
         dump_printf_loc.
         * gimple-fold.c: Likewise.
         * ipa-devirt.c: Likewise.
         * ipa-prop.c: Likewise.
         * ipa.c: Likewise.
         * tree-ssa-pre.c: Likewise.





[-- Attachment #2: devirt-location-fix.patch --]
[-- Type: text/x-patch, Size: 11003 bytes --]

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 76b2fda1..3b01718 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -905,12 +905,9 @@ walk_polymorphic_call_targets (pointer_set_t *reachable_call_targets,
 				 TDF_SLIM);
 	    }
           if (dump_enabled_p ())
-            {
-	      location_t locus = gimple_location (edge->call_stmt);
-	      dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, locus,
-			       "devirtualizing call in %s to %s\n",
-			       edge->caller->name (), target->name ());
-	    }
+	    dump_printf_loc_for_stmt (MSG_OPTIMIZED_LOCATIONS, edge->call_stmt,
+				      "devirtualizing call in %s to %s\n",
+				      edge->caller->name (), target->name ());
 
 	  cgraph_make_edge_direct (edge, target);
 	  cgraph_redirect_edge_call_stmt_to_callee (edge);
diff --git a/gcc/dumpfile.c b/gcc/dumpfile.c
index fd630a6..b7a791c 100644
--- a/gcc/dumpfile.c
+++ b/gcc/dumpfile.c
@@ -23,6 +23,12 @@ along with GCC; see the file COPYING3.  If not see
 #include "diagnostic-core.h"
 #include "dumpfile.h"
 #include "tree.h"
+#include "basic-block.h"
+#include "tree-ssa-alias.h"
+#include "internal-fn.h"
+#include "gimple-expr.h"
+#include "is-a.h"
+#include "gimple.h"
 #include "gimple-pretty-print.h"
 #include "context.h"
 
@@ -343,52 +349,80 @@ dump_generic_expr_loc (int dump_kind, source_location loc,
     }
 }
 
-/* Output a formatted message using FORMAT on appropriate dump streams.  */
+/* Output a formatted message using FORMAT on appropriate dump streams.
+   Accepts va_list AP as the last argument.  */
 
-void
-dump_printf (int dump_kind, const char *format, ...)
+ATTRIBUTE_NULL_PRINTF_2_0
+static void
+dump_vprintf (int dump_kind, const char *format, va_list ap)
 {
   if (dump_file && (dump_kind & pflags))
-    {
-      va_list ap;
-      va_start (ap, format);
       vfprintf (dump_file, format, ap);
-      va_end (ap);
-    }
 
   if (alt_dump_file && (dump_kind & alt_flags))
-    {
-      va_list ap;
-      va_start (ap, format);
       vfprintf (alt_dump_file, format, ap);
-      va_end (ap);
-    }
 }
 
-/* Similar to dump_printf, except source location is also printed.  */
+/* Output a formatted message using FORMAT on appropriate dump streams.  */
 
 void
-dump_printf_loc (int dump_kind, source_location loc, const char *format, ...)
+dump_printf (int dump_kind, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  dump_vprintf (dump_kind, format, ap);
+  va_end (ap);
+}
+
+/* Similar to dump_printf, except source location is also printed.
+   Accepts va_list AP as the last argument.  */
+
+void
+dump_vprintf_loc (int dump_kind, source_location loc, const char *format,
+		  va_list ap)
 {
   if (dump_file && (dump_kind & pflags))
     {
-      va_list ap;
       dump_loc (dump_kind, dump_file, loc);
-      va_start (ap, format);
       vfprintf (dump_file, format, ap);
-      va_end (ap);
     }
 
   if (alt_dump_file && (dump_kind & alt_flags))
     {
-      va_list ap;
       dump_loc (dump_kind, alt_dump_file, loc);
-      va_start (ap, format);
       vfprintf (alt_dump_file, format, ap);
-      va_end (ap);
     }
 }
 
+/* Similar to dump_printf, except source location is also printed.  */
+
+void
+dump_printf_loc (int dump_kind, source_location loc, const char *format, ...)
+{
+  va_list ap;
+  va_start (ap, format);
+  dump_vprintf_loc (dump_kind, loc, format, ap);
+  va_end (ap);
+}
+
+/* Similar to dump_printf, except source location is also printed if STMT
+   is not null. Otherwise, fallback to dump_fprintf is called.  */
+
+void
+dump_printf_loc_for_stmt (int dump_kind, const_gimple stmt, const char *format,
+			  ...)
+{
+  va_list ap;
+  va_start (ap, format);
+
+  if (stmt)
+    dump_vprintf_loc (dump_kind, gimple_location (stmt), format, ap);
+  else
+    dump_vprintf (dump_kind, format, ap);
+
+  va_end (ap);
+}
+
 /* Start a dump for PHASE. Store user-supplied dump flags in
    *FLAG_PTR.  Return the number of streams opened.  Set globals
    DUMP_FILE, and ALT_DUMP_FILE to point to the opened streams, and
diff --git a/gcc/dumpfile.h b/gcc/dumpfile.h
index 75949b7..5c1a177 100644
--- a/gcc/dumpfile.h
+++ b/gcc/dumpfile.h
@@ -126,8 +126,12 @@ extern void dump_end (int, FILE *);
 extern int opt_info_switch_p (const char *);
 extern const char *dump_flag_name (int);
 extern void dump_printf (int, const char *, ...) ATTRIBUTE_PRINTF_2;
+extern void dump_vprintf_loc (int, source_location, const char *,
+			      va_list ap) ATTRIBUTE_NULL_PRINTF_3_0;
 extern void dump_printf_loc (int, source_location,
                              const char *, ...) ATTRIBUTE_PRINTF_3;
+extern void dump_printf_loc_for_stmt (int, const_gimple,
+				      const char *, ...) ATTRIBUTE_PRINTF_3;
 extern void dump_basic_block (int, basic_block, int);
 extern void dump_generic_expr_loc (int, source_location, int, tree);
 extern void dump_generic_expr (int, int, tree);
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 403dee7..e831067 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -386,15 +386,13 @@ fold_gimple_assign (gimple_stmt_iterator *si)
 		    else
 		      fndecl = builtin_decl_implicit (BUILT_IN_UNREACHABLE);
 		    if (dump_enabled_p ())
-		      {
-			location_t loc = gimple_location (stmt);
-			dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc,
-					 "resolving virtual function address "
-					 "reference to function %s\n",
-					 targets.length () == 1
-					 ? targets[0]->name ()
-					 : "__builtin_unreachable");
-		      }
+		      dump_printf_loc_for_stmt (MSG_OPTIMIZED_LOCATIONS, stmt,
+						"resolving virtual function "
+						"address reference to "
+						"function %s\n",
+						targets.length () == 1
+						? targets[0]->name ()
+						: "__builtin_unreachable");
 		    val = fold_convert (TREE_TYPE (val), fndecl);
 		    STRIP_USELESS_TYPE_CONVERSION (val);
 		    return val;
@@ -1130,14 +1128,12 @@ gimple_fold_call (gimple_stmt_iterator *gsi, bool inplace)
 	    {
 	      tree lhs = gimple_call_lhs (stmt);
 	      if (dump_enabled_p ())
-		{
-		  location_t loc = gimple_location (stmt);
-		  dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc,
-				   "folding virtual function call to %s\n",
-		 		   targets.length () == 1
-		  		   ? targets[0]->name ()
-		  		   : "__builtin_unreachable");
-		}
+		dump_printf_loc_for_stmt (MSG_OPTIMIZED_LOCATIONS, stmt,
+					  "folding virtual function call "
+					  "to %s\n",
+					  targets.length () == 1
+					  ? targets[0]->name ()
+					  : "__builtin_unreachable");
 	      if (targets.length () == 1)
 		{
 		  gimple_call_set_fndecl (stmt, targets[0]->decl);
diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
index 21f4f11..facbf4c 100644
--- a/gcc/ipa-devirt.c
+++ b/gcc/ipa-devirt.c
@@ -2079,14 +2079,12 @@ ipa_devirt (void)
 	    else if (dbg_cnt (devirt))
 	      {
 		if (dump_enabled_p ())
-                  {
-                    location_t locus = gimple_location (e->call_stmt);
-                    dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, locus,
-                                     "speculatively devirtualizing call in %s/%i to %s/%i\n",
-                                     n->name (), n->order,
-                                     likely_target->name (),
-                                     likely_target->order);
-                  }
+		  dump_printf_loc_for_stmt (MSG_OPTIMIZED_LOCATIONS, e->call_stmt,
+					    "speculatively devirtualizing "
+					    "call in %s/%i to %s/%i\n",
+					    n->name (), n->order,
+					    likely_target->name (),
+					    likely_target->order);
 		if (!symtab_can_be_discarded (likely_target))
 		  {
 		    cgraph_node *alias;
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 5f5bf89..d062de8 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -2677,11 +2677,10 @@ ipa_make_edge_direct_to_target (struct cgraph_edge *ie, tree target)
 				"making it __builtin_unreachable\n";
 
 	      if (ie->call_stmt)
-		{
-		  location_t loc = gimple_location (ie->call_stmt);
-		  dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc, fmt,
-				   ie->caller->name (), ie->caller->order);
-		}
+		dump_printf_loc_for_stmt (MSG_OPTIMIZED_LOCATIONS,
+					  ie->call_stmt, fmt,
+					  ie->caller->name (),
+					  ie->caller->order);
 	      else if (dump_file)
 		fprintf (dump_file, fmt, ie->caller->name (), ie->caller->order);
 	    }
diff --git a/gcc/ipa.c b/gcc/ipa.c
index fce2e36..5c78690 100644
--- a/gcc/ipa.c
+++ b/gcc/ipa.c
@@ -197,14 +197,13 @@ walk_polymorphic_call_targets (pointer_set_t *reachable_call_targets,
 		       (builtin_decl_implicit (BUILT_IN_UNREACHABLE));
 
 	  if (dump_enabled_p ())
-            {
-              location_t locus = gimple_location (edge->call_stmt);
-              dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, locus,
-                               "devirtualizing call in %s/%i to %s/%i\n",
-                               edge->caller->name (), edge->caller->order,
-                               target->name (),
-                               target->order);
-	    }
+	    dump_printf_loc_for_stmt (MSG_OPTIMIZED_LOCATIONS, edge->call_stmt,
+				      "devirtualizing call in %s/%i to "
+				      "%s/%i\n",
+				      edge->caller->name (),
+				      edge->caller->order,
+				      target->name (),
+				      target->order);
 	  edge = cgraph_make_edge_direct (edge, target);
 	  if (inline_summary_vec)
 	    inline_update_overall_summary (node);
diff --git a/gcc/tree-ssa-pre.c b/gcc/tree-ssa-pre.c
index 74238de..e6a764e 100644
--- a/gcc/tree-ssa-pre.c
+++ b/gcc/tree-ssa-pre.c
@@ -4366,13 +4366,10 @@ eliminate_dom_walker::before_dom_children (basic_block b)
 	      if (fn && dbg_cnt (devirt))
 		{
 		  if (dump_enabled_p ())
-		    {
-		      location_t loc = gimple_location (stmt);
-		      dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc,
-				       "converting indirect call to "
-				       "function %s\n",
-				       cgraph_get_node (fn)->name ());
-		    }
+		    dump_printf_loc_for_stmt (MSG_OPTIMIZED_LOCATIONS, stmt,
+					      "converting indirect call to "
+					      "function %s\n",
+					      cgraph_get_node (fn)->name ());
 		  gimple_call_set_fndecl (stmt, fn);
 		  gimple_set_modified (stmt, true);
 		}
diff --git a/include/ansidecl.h b/include/ansidecl.h
index 0fb23bb..ba137aa 100644
--- a/include/ansidecl.h
+++ b/include/ansidecl.h
@@ -234,6 +234,12 @@ So instead we use the macro below and test it against specific values.  */
 # define ATTRIBUTE_NULL_PRINTF_3 ATTRIBUTE_NULL_PRINTF(3, 4)
 # define ATTRIBUTE_NULL_PRINTF_4 ATTRIBUTE_NULL_PRINTF(4, 5)
 # define ATTRIBUTE_NULL_PRINTF_5 ATTRIBUTE_NULL_PRINTF(5, 6)
+
+# define ATTRIBUTE_NULL_PRINTF_1_0 ATTRIBUTE_NULL_PRINTF(1, 0)
+# define ATTRIBUTE_NULL_PRINTF_2_0 ATTRIBUTE_NULL_PRINTF(2, 0)
+# define ATTRIBUTE_NULL_PRINTF_3_0 ATTRIBUTE_NULL_PRINTF(3, 0)
+# define ATTRIBUTE_NULL_PRINTF_4_0 ATTRIBUTE_NULL_PRINTF(4, 0)
+# define ATTRIBUTE_NULL_PRINTF_5_0 ATTRIBUTE_NULL_PRINTF(5, 0)
 #endif /* ATTRIBUTE_NULL_PRINTF */
 
 /* Attribute `sentinel' was valid as of gcc 3.5.  */

Re: [PATCH] Devirtualization dump functions fix

[Richard Biener]

On Thu, Jun 26, 2014 at 3:01 PM, Martin Liška <mliska@suse.cz> wrote:
> Hello,
>    I encountered similar issue to PR ipa/61462 where location_t locus =
> gimple_location (e->call_stmt) is called for e->call_stmt == NULL (Firefox
> with -flto -fdump-ipa-devirt). So that, I decided to introduce new function
> that is called for all potentially unsafe locations. I am wondering if a
> newly added function can be added in more seamless way (without playing with
> va_list and ATTRIBUTE_PRINTF stuff)?
>
> Bootstrapped and regtested on x86_64-unknown-linux-gnu.

Hmm, I don't like that very much - dump_printf_loc_for_stmt still implies
stmt is not NULL.  So you could have "fixed" gimple_location as well.
I suppose dump_printf_loc already does sth sane with UNKNOWN_LOCATION.

Richard.

> Thanks,
> Martin
>
>     ChangeLog:
>
>     2014-06-26  Martin Liska  <mliska@suse.cz>
>
>         * include/ansidecl.h: New collection of ATTRIBUTE_NULL_PRINTF_X_0
>         defined.
>
>     gcc/ChangeLog:
>
>     2014-06-26  Martin Liska  <mliska@suse.cz>
>
>         * dumpfile.h: New function dump_printf_loc_for_stmt.
>         * dumpfile.c: Implementation added.
>         (dump_vprintf): New function.i
>         * cgraphunit.c: dump_printf_loc_for_stmt usage replaces
>         dump_printf_loc.
>         * gimple-fold.c: Likewise.
>         * ipa-devirt.c: Likewise.
>         * ipa-prop.c: Likewise.
>         * ipa.c: Likewise.
>         * tree-ssa-pre.c: Likewise.
>
>
>
>

Re: [PATCH] Devirtualization dump functions fix

[Martin Liška]

On 06/26/2014 03:20 PM, Richard Biener wrote:
> On Thu, Jun 26, 2014 at 3:01 PM, Martin Liška <mliska@suse.cz> wrote:
>> Hello,
>>     I encountered similar issue to PR ipa/61462 where location_t locus =
>> gimple_location (e->call_stmt) is called for e->call_stmt == NULL (Firefox
>> with -flto -fdump-ipa-devirt). So that, I decided to introduce new function
>> that is called for all potentially unsafe locations. I am wondering if a
>> newly added function can be added in more seamless way (without playing with
>> va_list and ATTRIBUTE_PRINTF stuff)?
>>
>> Bootstrapped and regtested on x86_64-unknown-linux-gnu.
> Hmm, I don't like that very much - dump_printf_loc_for_stmt still implies
> stmt is not NULL.  So you could have "fixed" gimple_location as well.
> I suppose dump_printf_loc already does sth sane with UNKNOWN_LOCATION.
>
> Richard.
Hi,
    you are right that it is quite complex change.

Do you mean this one line change can be sufficient ?
diff --git a/gcc/gimple.h b/gcc/gimple.h
index ceefbc0..954195e 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -1498,7 +1498,7 @@ gimple_set_block (gimple g, tree block)
  static inline location_t
  gimple_location (const_gimple g)
  {
-  return g->location;
+  return g ? g->location : UNKNOWN_LOCATION;
  }

  /* Return pointer to location information for statement G.  */

I will double-check if it solves the problem ;)

Martin

>
>> Thanks,
>> Martin
>>
>>      ChangeLog:
>>
>>      2014-06-26  Martin Liska  <mliska@suse.cz>
>>
>>          * include/ansidecl.h: New collection of ATTRIBUTE_NULL_PRINTF_X_0
>>          defined.
>>
>>      gcc/ChangeLog:
>>
>>      2014-06-26  Martin Liska  <mliska@suse.cz>
>>
>>          * dumpfile.h: New function dump_printf_loc_for_stmt.
>>          * dumpfile.c: Implementation added.
>>          (dump_vprintf): New function.i
>>          * cgraphunit.c: dump_printf_loc_for_stmt usage replaces
>>          dump_printf_loc.
>>          * gimple-fold.c: Likewise.
>>          * ipa-devirt.c: Likewise.
>>          * ipa-prop.c: Likewise.
>>          * ipa.c: Likewise.
>>          * tree-ssa-pre.c: Likewise.
>>
>>
>>
>>

Re: [PATCH] Devirtualization dump functions fix

[Richard Biener]

On Thu, Jun 26, 2014 at 3:43 PM, Martin Liška <mliska@suse.cz> wrote:
>
> On 06/26/2014 03:20 PM, Richard Biener wrote:
>>
>> On Thu, Jun 26, 2014 at 3:01 PM, Martin Liška <mliska@suse.cz> wrote:
>>>
>>> Hello,
>>>     I encountered similar issue to PR ipa/61462 where location_t locus =
>>> gimple_location (e->call_stmt) is called for e->call_stmt == NULL
>>> (Firefox
>>> with -flto -fdump-ipa-devirt). So that, I decided to introduce new
>>> function
>>> that is called for all potentially unsafe locations. I am wondering if a
>>> newly added function can be added in more seamless way (without playing
>>> with
>>> va_list and ATTRIBUTE_PRINTF stuff)?
>>>
>>> Bootstrapped and regtested on x86_64-unknown-linux-gnu.
>>
>> Hmm, I don't like that very much - dump_printf_loc_for_stmt still implies
>> stmt is not NULL.  So you could have "fixed" gimple_location as well.
>> I suppose dump_printf_loc already does sth sane with UNKNOWN_LOCATION.
>>
>> Richard.
>
> Hi,
>    you are right that it is quite complex change.
>
> Do you mean this one line change can be sufficient ?
> diff --git a/gcc/gimple.h b/gcc/gimple.h
> index ceefbc0..954195e 100644
> --- a/gcc/gimple.h
> +++ b/gcc/gimple.h
> @@ -1498,7 +1498,7 @@ gimple_set_block (gimple g, tree block)
>  static inline location_t
>  gimple_location (const_gimple g)
>  {
> -  return g->location;
> +  return g ? g->location : UNKNOWN_LOCATION;
>  }
>
>  /* Return pointer to location information for statement G.  */
>
> I will double-check if it solves the problem ;)

Well yes - it is of course similar broken in spirit but at least a lot
simpler ;)  I'd put a comment there why we do check g for NULL.

Thanks,
Richard.

> Martin
>
>
>>
>>> Thanks,
>>> Martin
>>>
>>>      ChangeLog:
>>>
>>>      2014-06-26  Martin Liska  <mliska@suse.cz>
>>>
>>>          * include/ansidecl.h: New collection of
>>> ATTRIBUTE_NULL_PRINTF_X_0
>>>          defined.
>>>
>>>      gcc/ChangeLog:
>>>
>>>      2014-06-26  Martin Liska  <mliska@suse.cz>
>>>
>>>          * dumpfile.h: New function dump_printf_loc_for_stmt.
>>>          * dumpfile.c: Implementation added.
>>>          (dump_vprintf): New function.i
>>>          * cgraphunit.c: dump_printf_loc_for_stmt usage replaces
>>>          dump_printf_loc.
>>>          * gimple-fold.c: Likewise.
>>>          * ipa-devirt.c: Likewise.
>>>          * ipa-prop.c: Likewise.
>>>          * ipa.c: Likewise.
>>>          * tree-ssa-pre.c: Likewise.
>>>
>>>
>>>
>>>
>

Re: [PATCH] Devirtualization dump functions fix

[Jakub Jelinek]

On Thu, Jun 26, 2014 at 04:10:03PM +0200, Richard Biener wrote:
> On Thu, Jun 26, 2014 at 3:43 PM, Martin Liška <mliska@suse.cz> wrote:
> >
> > On 06/26/2014 03:20 PM, Richard Biener wrote:
> >>
> >> On Thu, Jun 26, 2014 at 3:01 PM, Martin Liška <mliska@suse.cz> wrote:
> >>>
> >>> Hello,
> >>>     I encountered similar issue to PR ipa/61462 where location_t locus =
> >>> gimple_location (e->call_stmt) is called for e->call_stmt == NULL
> >>> (Firefox
> >>> with -flto -fdump-ipa-devirt). So that, I decided to introduce new
> >>> function
> >>> that is called for all potentially unsafe locations. I am wondering if a
> >>> newly added function can be added in more seamless way (without playing
> >>> with
> >>> va_list and ATTRIBUTE_PRINTF stuff)?
> >>>
> >>> Bootstrapped and regtested on x86_64-unknown-linux-gnu.
> >>
> >> Hmm, I don't like that very much - dump_printf_loc_for_stmt still implies
> >> stmt is not NULL.  So you could have "fixed" gimple_location as well.
> >> I suppose dump_printf_loc already does sth sane with UNKNOWN_LOCATION.
> >>
> >> Richard.
> >
> > Hi,
> >    you are right that it is quite complex change.
> >
> > Do you mean this one line change can be sufficient ?
> > diff --git a/gcc/gimple.h b/gcc/gimple.h
> > index ceefbc0..954195e 100644
> > --- a/gcc/gimple.h
> > +++ b/gcc/gimple.h
> > @@ -1498,7 +1498,7 @@ gimple_set_block (gimple g, tree block)
> >  static inline location_t
> >  gimple_location (const_gimple g)
> >  {
> > -  return g->location;
> > +  return g ? g->location : UNKNOWN_LOCATION;
> >  }
> >
> >  /* Return pointer to location information for statement G.  */
> >
> > I will double-check if it solves the problem ;)
> 
> Well yes - it is of course similar broken in spirit but at least a lot
> simpler ;)  I'd put a comment there why we do check g for NULL.

But it increases overhead, there are hundreds of gimple_location calls
and most of them will never pass NULL.  Can't you simply
do what you do in the inline here in the couple of spots where
the stmt might be NULL?

	Jakub

Re: [PATCH] Devirtualization dump functions fix

[Martin Liška]

On 06/26/2014 04:18 PM, Jakub Jelinek wrote:
> On Thu, Jun 26, 2014 at 04:10:03PM +0200, Richard Biener wrote:
>> On Thu, Jun 26, 2014 at 3:43 PM, Martin Liška <mliska@suse.cz> wrote:
>>> On 06/26/2014 03:20 PM, Richard Biener wrote:
>>>> On Thu, Jun 26, 2014 at 3:01 PM, Martin Liška <mliska@suse.cz> wrote:
>>>>> Hello,
>>>>>      I encountered similar issue to PR ipa/61462 where location_t locus =
>>>>> gimple_location (e->call_stmt) is called for e->call_stmt == NULL
>>>>> (Firefox
>>>>> with -flto -fdump-ipa-devirt). So that, I decided to introduce new
>>>>> function
>>>>> that is called for all potentially unsafe locations. I am wondering if a
>>>>> newly added function can be added in more seamless way (without playing
>>>>> with
>>>>> va_list and ATTRIBUTE_PRINTF stuff)?
>>>>>
>>>>> Bootstrapped and regtested on x86_64-unknown-linux-gnu.
>>>> Hmm, I don't like that very much - dump_printf_loc_for_stmt still implies
>>>> stmt is not NULL.  So you could have "fixed" gimple_location as well.
>>>> I suppose dump_printf_loc already does sth sane with UNKNOWN_LOCATION.
>>>>
>>>> Richard.
>>> Hi,
>>>     you are right that it is quite complex change.
>>>
>>> Do you mean this one line change can be sufficient ?
>>> diff --git a/gcc/gimple.h b/gcc/gimple.h
>>> index ceefbc0..954195e 100644
>>> --- a/gcc/gimple.h
>>> +++ b/gcc/gimple.h
>>> @@ -1498,7 +1498,7 @@ gimple_set_block (gimple g, tree block)
>>>   static inline location_t
>>>   gimple_location (const_gimple g)
>>>   {
>>> -  return g->location;
>>> +  return g ? g->location : UNKNOWN_LOCATION;
>>>   }
>>>
>>>   /* Return pointer to location information for statement G.  */
>>>
>>> I will double-check if it solves the problem ;)
>> Well yes - it is of course similar broken in spirit but at least a lot
>> simpler ;)  I'd put a comment there why we do check g for NULL.
> But it increases overhead, there are hundreds of gimple_location calls
> and most of them will never pass NULL.  Can't you simply
> do what you do in the inline here in the couple of spots where
> the stmt might be NULL?
Sure, do you have any suggestion how should be called such function?
Suggestion: gimple_location_or_unknown ?

Thanks,
Martin

>
> 	Jakub

Re: [PATCH] Devirtualization dump functions fix

[Jakub Jelinek]

On Thu, Jun 26, 2014 at 04:27:49PM +0200, Martin Liška wrote:
> >>Well yes - it is of course similar broken in spirit but at least a lot
> >>simpler ;)  I'd put a comment there why we do check g for NULL.
> >But it increases overhead, there are hundreds of gimple_location calls
> >and most of them will never pass NULL.  Can't you simply
> >do what you do in the inline here in the couple of spots where
> >the stmt might be NULL?
> Sure, do you have any suggestion how should be called such function?
> Suggestion: gimple_location_or_unknown ?

gimple_location_safe or gimple_safe_location?

	Jakub

Re: [PATCH] Devirtualization dump functions fix

[Martin Liška]

[-- Attachment #1: Type: text/plain, Size: 1357 bytes --]


On 06/26/2014 04:29 PM, Jakub Jelinek wrote:
> On Thu, Jun 26, 2014 at 04:27:49PM +0200, Martin Liška wrote:
>>>> Well yes - it is of course similar broken in spirit but at least a lot
>>>> simpler ;)  I'd put a comment there why we do check g for NULL.
>>> But it increases overhead, there are hundreds of gimple_location calls
>>> and most of them will never pass NULL.  Can't you simply
>>> do what you do in the inline here in the couple of spots where
>>> the stmt might be NULL?
>> Sure, do you have any suggestion how should be called such function?
>> Suggestion: gimple_location_or_unknown ?
> gimple_location_safe or gimple_safe_location?
>
> 	Jakub
Thanks, there's new patch.

Patch has been tested for Firefox with -flto -fdump-ipa-devirt.
Bootstrap and regression tests have been running.

Ready for trunk after regression tests?

ChangeLog:

2014-06-26  Martin Liska  <mliska@suse.cz>

     * gimple.h (gimple_safe_location): New function introduced.
     * cgraphunit.c (walk_polymorphic_call_targets): Usage
     of gimple_safe_location replaces gimple_location.
     (gimple_fold_call): Likewise.
     * ipa-devirt.c (ipa_devirt): Likewise.
     * ipa-prop.c (ipa_make_edge_direct_to_target): Likewise.
     * ipa.c (walk_polymorphic_call_targets): Likewise.
     * tree-ssa-pre.c (eliminate_dom_walker::before_dom_children): Likewise.

[-- Attachment #2: devirt-location-fix3.patch --]
[-- Type: text/x-patch, Size: 5532 bytes --]

diff --git a/gcc/cgraphunit.c b/gcc/cgraphunit.c
index 76b2fda1..2bf5216 100644
--- a/gcc/cgraphunit.c
+++ b/gcc/cgraphunit.c
@@ -906,7 +906,7 @@ walk_polymorphic_call_targets (pointer_set_t *reachable_call_targets,
 	    }
           if (dump_enabled_p ())
             {
-	      location_t locus = gimple_location (edge->call_stmt);
+	      location_t locus = gimple_safe_location (edge->call_stmt);
 	      dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, locus,
 			       "devirtualizing call in %s to %s\n",
 			       edge->caller->name (), target->name ());
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index 403dee7..ad230be 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -387,7 +387,7 @@ fold_gimple_assign (gimple_stmt_iterator *si)
 		      fndecl = builtin_decl_implicit (BUILT_IN_UNREACHABLE);
 		    if (dump_enabled_p ())
 		      {
-			location_t loc = gimple_location (stmt);
+			location_t loc = gimple_safe_location (stmt);
 			dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc,
 					 "resolving virtual function address "
 					 "reference to function %s\n",
@@ -1131,7 +1131,7 @@ gimple_fold_call (gimple_stmt_iterator *gsi, bool inplace)
 	      tree lhs = gimple_call_lhs (stmt);
 	      if (dump_enabled_p ())
 		{
-		  location_t loc = gimple_location (stmt);
+		  location_t loc = gimple_safe_location (stmt);
 		  dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc,
 				   "folding virtual function call to %s\n",
 		 		   targets.length () == 1
diff --git a/gcc/gimple.h b/gcc/gimple.h
index ceefbc0..d401d47 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -1501,6 +1501,15 @@ gimple_location (const_gimple g)
   return g->location;
 }
 
+/* Return location information for statement G if g is not NULL.
+   Otherwise, UNKNOWN_LOCATION is returned.  */
+
+static inline location_t
+gimple_safe_location (const_gimple g)
+{
+  return g ? gimple_location (g) : UNKNOWN_LOCATION;
+}
+
 /* Return pointer to location information for statement G.  */
 
 static inline const location_t *
diff --git a/gcc/ipa-devirt.c b/gcc/ipa-devirt.c
index 21f4f11..4e5dae8 100644
--- a/gcc/ipa-devirt.c
+++ b/gcc/ipa-devirt.c
@@ -2080,7 +2080,7 @@ ipa_devirt (void)
 	      {
 		if (dump_enabled_p ())
                   {
-                    location_t locus = gimple_location (e->call_stmt);
+                    location_t locus = gimple_safe_location (e->call_stmt);
                     dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, locus,
                                      "speculatively devirtualizing call in %s/%i to %s/%i\n",
                                      n->name (), n->order,
diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
index 1e10b53..c6967be 100644
--- a/gcc/ipa-prop.c
+++ b/gcc/ipa-prop.c
@@ -2673,17 +2673,11 @@ ipa_make_edge_direct_to_target (struct cgraph_edge *ie, tree target)
 
           if (dump_enabled_p ())
 	    {
-	      const char *fmt = "discovered direct call to non-function in %s/%i, "
-				"making it __builtin_unreachable\n";
-
-	      if (ie->call_stmt)
-		{
-		  location_t loc = gimple_location (ie->call_stmt);
-		  dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc, fmt,
-				   ie->caller->name (), ie->caller->order);
-		}
-	      else if (dump_file)
-		fprintf (dump_file, fmt, ie->caller->name (), ie->caller->order);
+	      location_t loc = gimple_safe_location (ie->call_stmt);
+	      dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc,
+			       "discovered direct call to non-function in %s/%i, "
+			       "making it __builtin_unreachable\n",
+			       ie->caller->name (), ie->caller->order);
 	    }
 
 	  target = builtin_decl_implicit (BUILT_IN_UNREACHABLE);
@@ -2745,18 +2739,11 @@ ipa_make_edge_direct_to_target (struct cgraph_edge *ie, tree target)
      }
   if (dump_enabled_p ())
     {
-      const char *fmt = "converting indirect call in %s to direct call to %s\n";
-
-      if (ie->call_stmt)
-	{
-	  location_t loc = gimple_location (ie->call_stmt);
+      location_t loc = gimple_safe_location (ie->call_stmt);
 
-	  dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc, fmt,
-			   ie->caller->name (), callee->name ());
-
-	}
-      else if (dump_file)
-	fprintf (dump_file, fmt, ie->caller->name (), callee->name ());
+      dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc,
+		       "converting indirect call in %s to direct call to %s\n",
+		       ie->caller->name (), callee->name ());
     }
   ie = cgraph_make_edge_direct (ie, callee);
   es = inline_edge_summary (ie);
diff --git a/gcc/ipa.c b/gcc/ipa.c
index fce2e36..e3cd1a6 100644
--- a/gcc/ipa.c
+++ b/gcc/ipa.c
@@ -198,7 +198,7 @@ walk_polymorphic_call_targets (pointer_set_t *reachable_call_targets,
 
 	  if (dump_enabled_p ())
             {
-              location_t locus = gimple_location (edge->call_stmt);
+              location_t locus = gimple_safe_location (edge->call_stmt);
               dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, locus,
                                "devirtualizing call in %s/%i to %s/%i\n",
                                edge->caller->name (), edge->caller->order,
diff --git a/gcc/tree-ssa-pre.c b/gcc/tree-ssa-pre.c
index 74238de..b6ba309 100644
--- a/gcc/tree-ssa-pre.c
+++ b/gcc/tree-ssa-pre.c
@@ -4367,7 +4367,7 @@ eliminate_dom_walker::before_dom_children (basic_block b)
 		{
 		  if (dump_enabled_p ())
 		    {
-		      location_t loc = gimple_location (stmt);
+		      location_t loc = gimple_safe_location (stmt);
 		      dump_printf_loc (MSG_OPTIMIZED_LOCATIONS, loc,
 				       "converting indirect call to "
 				       "function %s\n",

Re: [PATCH] Devirtualization dump functions fix

[Richard Biener]

On Thu, Jun 26, 2014 at 5:58 PM, Martin Liška <mliska@suse.cz> wrote:
>
> On 06/26/2014 04:29 PM, Jakub Jelinek wrote:
>>
>> On Thu, Jun 26, 2014 at 04:27:49PM +0200, Martin Liška wrote:
>>>>>
>>>>> Well yes - it is of course similar broken in spirit but at least a lot
>>>>> simpler ;)  I'd put a comment there why we do check g for NULL.
>>>>
>>>> But it increases overhead, there are hundreds of gimple_location calls
>>>> and most of them will never pass NULL.  Can't you simply
>>>> do what you do in the inline here in the couple of spots where
>>>> the stmt might be NULL?
>>>
>>> Sure, do you have any suggestion how should be called such function?
>>> Suggestion: gimple_location_or_unknown ?
>>
>> gimple_location_safe or gimple_safe_location?
>>
>>         Jakub
>
> Thanks, there's new patch.
>
> Patch has been tested for Firefox with -flto -fdump-ipa-devirt.
> Bootstrap and regression tests have been running.
>
> Ready for trunk after regression tests?

Ok with s/gimple_safe_location/gimple_location_safe/ (I think that's
the more canonical naming - what's a "safe location" after all?)

Thanks,
Richard.

>
> ChangeLog:
>
> 2014-06-26  Martin Liska  <mliska@suse.cz>
>
>     * gimple.h (gimple_safe_location): New function introduced.
>     * cgraphunit.c (walk_polymorphic_call_targets): Usage
>     of gimple_safe_location replaces gimple_location.
>     (gimple_fold_call): Likewise.
>     * ipa-devirt.c (ipa_devirt): Likewise.
>     * ipa-prop.c (ipa_make_edge_direct_to_target): Likewise.
>     * ipa.c (walk_polymorphic_call_targets): Likewise.
>     * tree-ssa-pre.c (eliminate_dom_walker::before_dom_children): Likewise.

Re: [PATCH] Devirtualization dump functions fix

[Martin Liška]

On 06/27/2014 10:38 AM, Richard Biener wrote:
> On Thu, Jun 26, 2014 at 5:58 PM, Martin Liška <mliska@suse.cz> wrote:
>> On 06/26/2014 04:29 PM, Jakub Jelinek wrote:
>>> On Thu, Jun 26, 2014 at 04:27:49PM +0200, Martin Liška wrote:
>>>>>> Well yes - it is of course similar broken in spirit but at least a lot
>>>>>> simpler ;)  I'd put a comment there why we do check g for NULL.
>>>>> But it increases overhead, there are hundreds of gimple_location calls
>>>>> and most of them will never pass NULL.  Can't you simply
>>>>> do what you do in the inline here in the couple of spots where
>>>>> the stmt might be NULL?
>>>> Sure, do you have any suggestion how should be called such function?
>>>> Suggestion: gimple_location_or_unknown ?
>>> gimple_location_safe or gimple_safe_location?
>>>
>>>          Jakub
>> Thanks, there's new patch.
>>
>> Patch has been tested for Firefox with -flto -fdump-ipa-devirt.
>> Bootstrap and regression tests have been running.
>>
>> Ready for trunk after regression tests?
> Ok with s/gimple_safe_location/gimple_location_safe/ (I think that's
> the more canonical naming - what's a "safe location" after all?)
>
> Thanks,
> Richard.
You are right, gimple_location_safe sounds better.
Patch has been just committed with your change.

Thanks,
Martin
>
>> ChangeLog:
>>
>> 2014-06-26  Martin Liska  <mliska@suse.cz>
>>
>>      * gimple.h (gimple_safe_location): New function introduced.
>>      * cgraphunit.c (walk_polymorphic_call_targets): Usage
>>      of gimple_safe_location replaces gimple_location.
>>      (gimple_fold_call): Likewise.
>>      * ipa-devirt.c (ipa_devirt): Likewise.
>>      * ipa-prop.c (ipa_make_edge_direct_to_target): Likewise.
>>      * ipa.c (walk_polymorphic_call_targets): Likewise.
>>      * tree-ssa-pre.c (eliminate_dom_walker::before_dom_children): Likewise.