diff mbox

[2/2] Reducing the overhead of dwarf2 location tracking

Message ID 87mxl9vdj0.fsf@firetop.home
State New
Headers show

Commit Message

Richard Sandiford March 5, 2011, 2:37 p.m. UTC
Jakub Jelinek <jakub@redhat.com> writes:
> On Fri, Mar 04, 2011 at 01:56:55PM +0000, Richard Sandiford wrote:
>> 	* dwarf2out.c (dw_loc_list_node): Add resolved_addr and replaced.
>> 	(cached_dw_loc_list_def): New structure.
>> 	(cached_dw_loc_list): New typedef.
>> 	(cached_dw_loc_list_table): New variable.
>> 	(cached_dw_loc_list_table_hash): New function.
>> 	(cached_dw_loc_list_table_eq): Likewise.
>> 	(add_location_or_const_value_attribute): Take a bool cache_p.
>> 	Cache the list when the parameter is true.
>> 	(gen_formal_parameter_die): Update caller.
>> 	(gen_variable_die): Likewise.
>> 	(dwarf2out_finish): Likewise.
>> 	(dwarf2out_function_decl): Clear cached_dw_loc_list_table.
>> 	(dwarf2out_init): Initialize cached_dw_loc_list_table.
>> 	(resolve_addr): Cache the result of resolving a chain of
>> 	location lists.
>
> I think you should handle the cached_dw_loc_list_table in
> dwarf2out_abstract_function similarly to say decl_loc_table, i.e.
> save/clear for the duration of the of recursive dwarf2out_decl
> call, restore afterwards and in the places where you actually use
> it guard it also with cached_dw_loc_list_table != NULL.

OK, thanks for the pointer.  How does this look?  Bootstrapped
& regression-tested on x86_64-linux-gnu.

Richard


gcc/
	* dwarf2out.c (dw_loc_list_node): Add resolved_addr and replaced.
	(cached_dw_loc_list_def): New structure.
	(cached_dw_loc_list): New typedef.
	(cached_dw_loc_list_table): New variable.
	(cached_dw_loc_list_table_hash): New function.
	(cached_dw_loc_list_table_eq): Likewise.
	(add_location_or_const_value_attribute): Take a bool cache_p.
	Cache the list when the parameter is true.
	(gen_formal_parameter_die): Update caller.
	(gen_variable_die): Likewise.
	(dwarf2out_finish): Likewise.
	(dwarf2out_abstract_function): Nullify cached_dw_loc_list_table
	while generating debug info for the decl.
	(dwarf2out_function_decl): Clear cached_dw_loc_list_table.
	(dwarf2out_init): Initialize cached_dw_loc_list_table.
	(resolve_addr): Cache the result of resolving a chain of
	location lists.

Comments

Richard Sandiford April 4, 2011, 10:49 a.m. UTC | #1
Richard Sandiford <rdsandiford@googlemail.com> writes:
> gcc/
> 	* dwarf2out.c (dw_loc_list_node): Add resolved_addr and replaced.
> 	(cached_dw_loc_list_def): New structure.
> 	(cached_dw_loc_list): New typedef.
> 	(cached_dw_loc_list_table): New variable.
> 	(cached_dw_loc_list_table_hash): New function.
> 	(cached_dw_loc_list_table_eq): Likewise.
> 	(add_location_or_const_value_attribute): Take a bool cache_p.
> 	Cache the list when the parameter is true.
> 	(gen_formal_parameter_die): Update caller.
> 	(gen_variable_die): Likewise.
> 	(dwarf2out_finish): Likewise.
> 	(dwarf2out_abstract_function): Nullify cached_dw_loc_list_table
> 	while generating debug info for the decl.
> 	(dwarf2out_function_decl): Clear cached_dw_loc_list_table.
> 	(dwarf2out_init): Initialize cached_dw_loc_list_table.
> 	(resolve_addr): Cache the result of resolving a chain of
> 	location lists.

This has been on mainline for a week without any reported problems (AFAIK).
Is it OK for 4.5 and 4.6 as well?

Thanks,
Richard
diff mbox

Patch

Index: gcc/dwarf2out.c
===================================================================
--- gcc/dwarf2out.c	2011-03-05 09:02:43.000000000 +0000
+++ gcc/dwarf2out.c	2011-03-05 09:09:46.000000000 +0000
@@ -4464,6 +4464,11 @@  typedef struct GTY(()) dw_loc_list_struc
   const char *section; /* Section this loclist is relative to */
   dw_loc_descr_ref expr;
   hashval_t hash;
+  /* True if all addresses in this and subsequent lists are known to be
+     resolved.  */
+  bool resolved_addr;
+  /* True if this list has been replaced by dw_loc_next.  */
+  bool replaced;
   bool emitted;
 } dw_loc_list_node;
 
@@ -6119,6 +6124,19 @@  typedef struct var_loc_list_def var_loc_
 /* Table of decl location linked lists.  */
 static GTY ((param_is (var_loc_list))) htab_t decl_loc_table;
 
+/* A cached location list.  */
+struct GTY (()) cached_dw_loc_list_def {
+  /* The DECL_UID of the decl that this entry describes.  */
+  unsigned int decl_id;
+
+  /* The cached location list.  */
+  dw_loc_list_ref loc_list;
+};
+typedef struct cached_dw_loc_list_def cached_dw_loc_list;
+
+/* Table of cached location lists.  */
+static GTY ((param_is (cached_dw_loc_list))) htab_t cached_dw_loc_list_table;
+
 /* A pointer to the base of a list of references to DIE's that
    are uniquely identified by their tag, presence/absence of
    children DIE's, and list of attribute/value pairs.  */
@@ -6479,7 +6497,7 @@  static void insert_int (HOST_WIDE_INT, u
 static void insert_double (double_int, unsigned char *);
 static void insert_float (const_rtx, unsigned char *);
 static rtx rtl_for_decl_location (tree);
-static bool add_location_or_const_value_attribute (dw_die_ref, tree,
+static bool add_location_or_const_value_attribute (dw_die_ref, tree, bool,
 						   enum dwarf_attribute);
 static bool tree_add_const_value_attribute (dw_die_ref, tree);
 static bool tree_add_const_value_attribute_for_decl (dw_die_ref, tree);
@@ -8201,6 +8219,24 @@  lookup_decl_loc (const_tree decl)
     htab_find_with_hash (decl_loc_table, decl, DECL_UID (decl));
 }
 
+/* Returns a hash value for X (which really is a cached_dw_loc_list_list).  */
+
+static hashval_t
+cached_dw_loc_list_table_hash (const void *x)
+{
+  return (hashval_t) ((const cached_dw_loc_list *) x)->decl_id;
+}
+
+/* Return nonzero if decl_id of cached_dw_loc_list X is the same as
+   UID of decl *Y.  */
+
+static int
+cached_dw_loc_list_table_eq (const void *x, const void *y)
+{
+  return (((const cached_dw_loc_list *) x)->decl_id
+	  == DECL_UID ((const_tree) y));
+}
+
 /* Equate a DIE to a particular declaration.  */
 
 static void
@@ -16978,15 +17014,22 @@  fortran_common (tree decl, HOST_WIDE_INT
    these things can crop up in other ways also.)  Note that one type of
    constant value which can be passed into an inlined function is a constant
    pointer.  This can happen for example if an actual argument in an inlined
-   function call evaluates to a compile-time constant address.  */
+   function call evaluates to a compile-time constant address.
+
+   CACHE_P is true if it is worth caching the location list for DECL,
+   so that future calls can reuse it rather than regenerate it from scratch.
+   This is true for BLOCK_NONLOCALIZED_VARS in inlined subroutines,
+   since we will need to refer to them each time the function is inlined.  */
 
 static bool
-add_location_or_const_value_attribute (dw_die_ref die, tree decl,
+add_location_or_const_value_attribute (dw_die_ref die, tree decl, bool cache_p,
 				       enum dwarf_attribute attr)
 {
   rtx rtl;
   dw_loc_list_ref list;
   var_loc_list *loc_list;
+  cached_dw_loc_list *cache;
+  void **slot;
 
   if (TREE_CODE (decl) == ERROR_MARK)
     return false;
@@ -17023,7 +17066,33 @@  add_location_or_const_value_attribute (d
 	  && add_const_value_attribute (die, rtl))
 	 return true;
     }
-  list = loc_list_from_tree (decl, decl_by_reference_p (decl) ? 0 : 2);
+  /* If this decl is from BLOCK_NONLOCALIZED_VARS, we might need its
+     list several times.  See if we've already cached the contents.  */
+  list = NULL;
+  if (loc_list == NULL || cached_dw_loc_list_table == NULL)
+    cache_p = false;
+  if (cache_p)
+    {
+      cache = (cached_dw_loc_list *)
+	htab_find_with_hash (cached_dw_loc_list_table, decl, DECL_UID (decl));
+      if (cache)
+	list = cache->loc_list;
+    }
+  if (list == NULL)
+    {
+      list = loc_list_from_tree (decl, decl_by_reference_p (decl) ? 0 : 2);
+      /* It is usually worth caching this result if the decl is from
+	 BLOCK_NONLOCALIZED_VARS and if the list has at least two elements.  */
+      if (cache_p && list && list->dw_loc_next)
+	{
+	  slot = htab_find_slot_with_hash (cached_dw_loc_list_table, decl,
+					   DECL_UID (decl), INSERT);
+	  cache = ggc_alloc_cleared_cached_dw_loc_list ();
+	  cache->decl_id = DECL_UID (decl);
+	  cache->loc_list = list;
+	  *slot = cache;
+	}
+    }
   if (list)
     {
       add_AT_location_description (die, attr, list);
@@ -18684,7 +18753,7 @@  gen_formal_parameter_die (tree node, tre
         equate_decl_number_to_die (node, parm_die);
       if (! DECL_ABSTRACT (node_or_origin))
 	add_location_or_const_value_attribute (parm_die, node_or_origin,
-					       DW_AT_location);
+					       node == NULL, DW_AT_location);
 
       break;
 
@@ -18869,6 +18938,7 @@  dwarf2out_abstract_function (tree decl)
   tree context;
   int was_abstract;
   htab_t old_decl_loc_table;
+  htab_t old_cached_dw_loc_list_table;
 
   /* Make sure we have the actual abstract inline, not a clone.  */
   decl = DECL_ORIGIN (decl);
@@ -18882,7 +18952,9 @@  dwarf2out_abstract_function (tree decl)
      DIE.  Be sure to not clobber the outer location table nor use it or we would
      get locations in abstract instantces.  */
   old_decl_loc_table = decl_loc_table;
+  old_cached_dw_loc_list_table = cached_dw_loc_list_table;
   decl_loc_table = NULL;
+  cached_dw_loc_list_table = NULL;
 
   /* Be sure we've emitted the in-class declaration DIE (if any) first, so
      we don't get confused by DECL_ABSTRACT.  */
@@ -18907,6 +18979,7 @@  dwarf2out_abstract_function (tree decl)
 
   current_function_decl = save_fn;
   decl_loc_table = old_decl_loc_table;
+  cached_dw_loc_list_table = old_cached_dw_loc_list_table;
   pop_cfun ();
 }
 
@@ -19723,9 +19796,8 @@  gen_variable_die (tree decl, tree origin
           && !TREE_SYMBOL_REFERENCED (DECL_ASSEMBLER_NAME (decl_or_origin)))
 	defer_location (decl_or_origin, var_die);
       else
-        add_location_or_const_value_attribute (var_die,
-					       decl_or_origin,
-					       DW_AT_location);
+        add_location_or_const_value_attribute (var_die, decl_or_origin,
+					       decl == NULL, DW_AT_location);
       add_pubname (decl_or_origin, var_die);
     }
   else
@@ -21501,6 +21573,7 @@  dwarf2out_function_decl (tree decl)
   dwarf2out_decl (decl);
 
   htab_empty (decl_loc_table);
+  htab_empty (cached_dw_loc_list_table);
 }
 
 /* Output a marker (i.e. a label) for the beginning of the generated code for
@@ -22210,6 +22283,11 @@  dwarf2out_init (const char *filename ATT
   decl_loc_table = htab_create_ggc (10, decl_loc_table_hash,
 				    decl_loc_table_eq, NULL);
 
+  /* Allocate the cached_dw_loc_list_table.  */
+  cached_dw_loc_list_table
+    = htab_create_ggc (10, cached_dw_loc_list_table_hash,
+		       cached_dw_loc_list_table_eq, NULL);
+
   /* Allocate the initial hunk of the decl_scope_table.  */
   decl_scope_table = VEC_alloc (tree, gc, 256);
 
@@ -22852,30 +22930,53 @@  resolve_addr (dw_die_ref die)
 {
   dw_die_ref c;
   dw_attr_ref a;
-  dw_loc_list_ref *curr;
+  dw_loc_list_ref *curr, *start, loc;
   unsigned ix;
 
   FOR_EACH_VEC_ELT (dw_attr_node, die->die_attr, ix, a)
     switch (AT_class (a))
       {
       case dw_val_class_loc_list:
-	curr = AT_loc_list_ptr (a);
-	while (*curr)
+	start = curr = AT_loc_list_ptr (a);
+	loc = *curr;
+	gcc_assert (loc);
+	/* The same list can be referenced more than once.  See if we have
+	   already recorded the result from a previous pass.  */
+	if (loc->replaced)
+	  *curr = loc->dw_loc_next;
+	else if (!loc->resolved_addr)
 	  {
-	    if (!resolve_addr_in_expr ((*curr)->expr))
+	    /* As things stand, we do not expect or allow one die to
+	       reference a suffix of another die's location list chain.
+	       References must be identical or completely separate.
+	       There is therefore no need to cache the result of this
+	       pass on any list other than the first; doing so
+	       would lead to unnecessary writes.  */
+	    while (*curr)
 	      {
-		dw_loc_list_ref next = (*curr)->dw_loc_next;
-		if (next && (*curr)->ll_symbol)
+		gcc_assert (!(*curr)->replaced && !(*curr)->resolved_addr);
+		if (!resolve_addr_in_expr ((*curr)->expr))
 		  {
-		    gcc_assert (!next->ll_symbol);
-		    next->ll_symbol = (*curr)->ll_symbol;
+		    dw_loc_list_ref next = (*curr)->dw_loc_next;
+		    if (next && (*curr)->ll_symbol)
+		      {
+			gcc_assert (!next->ll_symbol);
+			next->ll_symbol = (*curr)->ll_symbol;
+		      }
+		    *curr = next;
 		  }
-		*curr = next;
+		else
+		  curr = &(*curr)->dw_loc_next;
 	      }
+	    if (loc == *start)
+	      loc->resolved_addr = 1;
 	    else
-	      curr = &(*curr)->dw_loc_next;
+	      {
+		loc->replaced = 1;
+		loc->dw_loc_next = *start;
+	      }
 	  }
-	if (!AT_loc_list (a))
+	if (!*start)
 	  {
 	    remove_AT (die, a->dw_attr);
 	    ix--;
@@ -23304,6 +23405,7 @@  dwarf2out_finish (const char *filename)
       add_location_or_const_value_attribute (
         VEC_index (deferred_locations, deferred_locations_list, i)->die,
         VEC_index (deferred_locations, deferred_locations_list, i)->variable,
+	false,
 	DW_AT_location);
     }