Patchwork [PR,debug/47624] improve value tracking in non-VTA locations

login
register
mail settings
Submitter Alexandre Oliva
Date April 22, 2012, 6:08 p.m.
Message ID <or4nsbr654.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/154299/
State New
Headers show

Comments

Alexandre Oliva - April 22, 2012, 6:08 p.m.
On Jun  4, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:

> On Feb 15, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:
>> VTA only tracks locations of gimple regs, while addressable variables
>> still use the old var tracking strategy.  This means addressable
>> variables, during var-tracking, got locations that were not based on
>> VALUEs, which failed immediately in the presence of auto-inc addresses.
>> The locations also tended to degrade in other ways, when a register
>> holding an address happened to be overwritten at a later point.

>> This patch arranges for us to track addresses of these variables as
>> VALUEs, and to emit new locations for them when a location whose value
>> was used to compute its address changes, fixing the problem.

>> The patch was regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to
>> install?

> Ping?  Retested on both platforms, unchanged except for fixing the typo
> s/deug/debug/ in the ChangeLog entry, that Jakub caught.
> http://gcc.gnu.org/ml/gcc-patches/2011-02/msg00981.html

Ping?  Here's an updated patch that uses some of the recently-added
infrastructure to notify dependent variables and values when their
dependencies change.

It didn't look reasonable to add a field to all variables parts, or even
to all variables, just to hold a chain of loc_deps.  For one-part
variables, we reused cur_loc, but after some measurements I concluded it
was perfectly ok to leave some backlinks behind or dangling, checking
them on activation.

It's not even a significant waste of memory: compare-debug bootstraps on
x86_64- and i686-linux-gnu leave only 1 loc_exp_dep allocated in the
pool I created for NOT_ONEPART loc_exp_deps in 3447 of the 5172
functions that leave any loc_exp_deps dangling off dependent values, at
the end of var-tracking.  Of all these 5172 functions, only 4 ever went
beyond the initial pool allocation of 64 loc_exp_deps, which is less
than 0.05%, with 98 as the highest count on the bootstraps.  A 32
initial allocation would have been enough to cover 99.71% of the
functions that have backlinks left behind at the end of var-tracking
(thus released along with the pool), but since even 64 loc_exp_deps add
up to as little as 512 pointers (half a 4KB page), I decided to leave it
at that.

Regstrapped on x86_64- and i686-linux-gnu.  This ought to fix
gcc.dg/guality/pr43077-1.c -O1 on powerpc (which is what the bug report
is about), but I only verified that by examining a cross compiler
output.

Ok to install?
Alexandre Oliva - June 13, 2012, 8:02 a.m.
On Apr 22, 2012, Alexandre Oliva <aoliva@redhat.com> wrote:

> for  gcc/ChangeLog
> from  Alexandre Oliva  <aoliva@redhat.com>

> 	PR debug/47624
> 	* var-tracking.c (loc_exp_dep_pool): New.
> 	(vt_emit_notes): Create and release the pool.
> 	(compute_bb_dataflow): Use value-based locations in MO_VAL_SET.
> 	(emit_notes_in_bb): Likewise.
> 	(loc_exp_dep_insert): Deal with NOT_ONEPART vars.
> 	(notify_dependents_of_changed_value): Likewise.
> 	(notify_dependents_of_resolved_value): Check that NOT_ONEPART
> 	variables don't have a VAR_LOC_DEP_LST.
> 	(emit_note_insn_var_location): Expand NOT_ONEPART locs that are
> 	VALUEs or MEMs of VALUEs.

Ping?  http://gcc.gnu.org/ml/gcc-patches/2012-04/msg01320.html

RTH, the patch for PR49888 that you reviewed and approved depends on
this one.
Richard Henderson - June 13, 2012, 7:04 p.m.
On 2012-06-13 01:02, Alexandre Oliva wrote:
> Ping?  http://gcc.gnu.org/ml/gcc-patches/2012-04/msg01320.html

Ok.


r~

Patch

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/47624
	* var-tracking.c (loc_exp_dep_pool): New.
	(vt_emit_notes): Create and release the pool.
	(compute_bb_dataflow): Use value-based locations in MO_VAL_SET.
	(emit_notes_in_bb): Likewise.
	(loc_exp_dep_insert): Deal with NOT_ONEPART vars.
	(notify_dependents_of_changed_value): Likewise.
	(notify_dependents_of_resolved_value): Check that NOT_ONEPART
	variables don't have a VAR_LOC_DEP_LST.
	(emit_note_insn_var_location): Expand NOT_ONEPART locs that are
	VALUEs or MEMs of VALUEs.

Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c.orig	2012-04-13 12:54:10.000000000 -0300
+++ gcc/var-tracking.c	2012-04-18 03:47:48.000000000 -0300
@@ -474,6 +474,9 @@  static alloc_pool loc_chain_pool;
 /* Alloc pool for struct shared_hash_def.  */
 static alloc_pool shared_hash_pool;
 
+/* Alloc pool for struct loc_exp_dep_s for NOT_ONEPART variables.  */
+static alloc_pool loc_exp_dep_pool;
+
 /* Changed variables, notes will be emitted for them.  */
 static htab_t changed_variables;
 
@@ -6284,29 +6287,41 @@  compute_bb_dataflow (basic_block bb)
 	    {
 	      rtx loc = mo->u.loc;
 	      rtx val, vloc, uloc;
+	      rtx dstv, srcv;
 
 	      vloc = loc;
 	      uloc = XEXP (vloc, 1);
 	      val = XEXP (vloc, 0);
 	      vloc = uloc;
 
+	      if (GET_CODE (uloc) == SET)
+		{
+		  dstv = SET_DEST (uloc);
+		  srcv = SET_SRC (uloc);
+		}
+	      else
+		{
+		  dstv = uloc;
+		  srcv = NULL;
+		}
+
 	      if (GET_CODE (val) == CONCAT)
 		{
-		  vloc = XEXP (val, 1);
+		  dstv = vloc = XEXP (val, 1);
 		  val = XEXP (val, 0);
 		}
 
 	      if (GET_CODE (vloc) == SET)
 		{
-		  rtx vsrc = SET_SRC (vloc);
+		  srcv = SET_SRC (vloc);
 
-		  gcc_assert (val != vsrc);
+		  gcc_assert (val != srcv);
 		  gcc_assert (vloc == uloc || VAL_NEEDS_RESOLUTION (loc));
 
-		  vloc = SET_DEST (vloc);
+		  dstv = vloc = SET_DEST (vloc);
 
 		  if (VAL_NEEDS_RESOLUTION (loc))
-		    val_resolve (out, val, vsrc, insn);
+		    val_resolve (out, val, srcv, insn);
 		}
 	      else if (VAL_NEEDS_RESOLUTION (loc))
 		{
@@ -6322,45 +6337,53 @@  compute_bb_dataflow (basic_block bb)
 		      if (REG_P (uloc))
 			var_reg_delete (out, uloc, true);
 		      else if (MEM_P (uloc))
-			var_mem_delete (out, uloc, true);
+			{
+			  gcc_assert (MEM_P (dstv));
+			  gcc_assert (MEM_ATTRS (dstv) == MEM_ATTRS (uloc));
+			  var_mem_delete (out, dstv, true);
+			}
 		    }
 		  else
 		    {
 		      bool copied_p = VAL_EXPR_IS_COPIED (loc);
-		      rtx set_src = NULL;
+		      rtx src = NULL, dst = uloc;
 		      enum var_init_status status = VAR_INIT_STATUS_INITIALIZED;
 
 		      if (GET_CODE (uloc) == SET)
 			{
-			  set_src = SET_SRC (uloc);
-			  uloc = SET_DEST (uloc);
+			  src = SET_SRC (uloc);
+			  dst = SET_DEST (uloc);
 			}
 
 		      if (copied_p)
 			{
 			  if (flag_var_tracking_uninit)
 			    {
-			      status = find_src_status (in, set_src);
+			      status = find_src_status (in, src);
 
 			      if (status == VAR_INIT_STATUS_UNKNOWN)
-				status = find_src_status (out, set_src);
+				status = find_src_status (out, src);
 			    }
 
-			  set_src = find_src_set_src (in, set_src);
+			  src = find_src_set_src (in, src);
 			}
 
-		      if (REG_P (uloc))
-			var_reg_delete_and_set (out, uloc, !copied_p,
-						status, set_src);
-		      else if (MEM_P (uloc))
-			var_mem_delete_and_set (out, uloc, !copied_p,
-						status, set_src);
+		      if (REG_P (dst))
+			var_reg_delete_and_set (out, dst, !copied_p,
+						status, srcv);
+		      else if (MEM_P (dst))
+			{
+			  gcc_assert (MEM_P (dstv));
+			  gcc_assert (MEM_ATTRS (dstv) == MEM_ATTRS (dst));
+			  var_mem_delete_and_set (out, dstv, !copied_p,
+						  status, srcv);
+			}
 		    }
 		}
 	      else if (REG_P (uloc))
 		var_regno_delete (out, REGNO (uloc));
 
-	      val_store (out, val, vloc, insn, true);
+	      val_store (out, val, dstv, insn, true);
 	    }
 	    break;
 
@@ -7602,8 +7625,13 @@  loc_exp_insert_dep (variable var, rtx x,
   if (VAR_LOC_DEP_LST (xvar) && VAR_LOC_DEP_LST (xvar)->dv == var->dv)
     return;
 
-  VEC_quick_push (loc_exp_dep, VAR_LOC_DEP_VEC (var), NULL);
-  led = VEC_last (loc_exp_dep, VAR_LOC_DEP_VEC (var));
+  if (var->onepart == NOT_ONEPART)
+    led = (loc_exp_dep *) pool_alloc (loc_exp_dep_pool);
+  else
+    {
+      VEC_quick_push (loc_exp_dep, VAR_LOC_DEP_VEC (var), NULL);
+      led = VEC_last (loc_exp_dep, VAR_LOC_DEP_VEC (var));
+    }
   led->dv = var->dv;
   led->value = x;
 
@@ -7679,8 +7707,12 @@  notify_dependents_of_resolved_value (var
 
 	  gcc_checking_assert (dv_changed_p (dv));
 	}
-      else if (!dv_changed_p (dv))
-	continue;
+      else
+	{
+	  gcc_checking_assert (dv_onepart_p (dv) != NOT_ONEPART);
+	  if (!dv_changed_p (dv))
+	    continue;
+      }
 
       var = (variable) htab_find_with_hash (vars, dv, dv_htab_hash (dv));
 
@@ -8135,11 +8167,23 @@  emit_note_insn_var_location (void **varp
 	  else if (last_limit > VAR_PART_OFFSET (var, i))
 	    continue;
 	  offset = VAR_PART_OFFSET (var, i);
-	  if (!var->var_part[i].cur_loc)
+	  loc2 = var->var_part[i].cur_loc;
+	  if (loc2 && GET_CODE (loc2) == MEM
+	      && GET_CODE (XEXP (loc2, 0)) == VALUE)
+	    {
+	      rtx depval = XEXP (loc2, 0);
+
+	      loc2 = vt_expand_loc (loc2, vars);
+
+	      if (loc2)
+		loc_exp_insert_dep (var, depval, vars);
+	    }
+	  if (!loc2)
 	    {
 	      complete = false;
 	      continue;
 	    }
+	  gcc_checking_assert (GET_CODE (loc2) != VALUE);
 	  for (lc = var->var_part[i].loc_chain; lc; lc = lc->next)
 	    if (var->var_part[i].cur_loc == lc->loc)
 	      {
@@ -8147,7 +8191,6 @@  emit_note_insn_var_location (void **varp
 		break;
 	      }
 	  gcc_assert (lc);
-	  loc2 = var->var_part[i].cur_loc;
 	}
 
       offsets[n_var_parts] = offset;
@@ -8361,7 +8404,6 @@  notify_dependents_of_changed_value (rtx 
   while ((led = VAR_LOC_DEP_LST (var)))
     {
       decl_or_value ldv = led->dv;
-      void **islot;
       variable ivar;
 
       /* Deactivate and remove the backlink, as it was “used up”.  It
@@ -8386,13 +8428,34 @@  notify_dependents_of_changed_value (rtx 
 	  VEC_safe_push (rtx, stack, *changed_values_stack, dv_as_rtx (ldv));
 	  break;
 
-	default:
-	  islot = htab_find_slot_with_hash (htab, ldv, dv_htab_hash (ldv),
-					    NO_INSERT);
-	  ivar = (variable) *islot;
+	case ONEPART_VDECL:
+	  ivar = (variable) htab_find_with_hash (htab, ldv, dv_htab_hash (ldv));
 	  gcc_checking_assert (!VAR_LOC_DEP_LST (ivar));
 	  variable_was_changed (ivar, NULL);
 	  break;
+
+	case NOT_ONEPART:
+	  pool_free (loc_exp_dep_pool, led);
+	  ivar = (variable) htab_find_with_hash (htab, ldv, dv_htab_hash (ldv));
+	  if (ivar)
+	    {
+	      int i = ivar->n_var_parts;
+	      while (i--)
+		{
+		  rtx loc = ivar->var_part[i].cur_loc;
+
+		  if (loc && GET_CODE (loc) == MEM
+		      && XEXP (loc, 0) == val)
+		    {
+		      variable_was_changed (ivar, NULL);
+		      break;
+		    }
+		}
+	    }
+	  break;
+
+	default:
+	  gcc_unreachable ();
 	}
     }
 }
@@ -8729,29 +8792,41 @@  emit_notes_in_bb (basic_block bb, datafl
 	    {
 	      rtx loc = mo->u.loc;
 	      rtx val, vloc, uloc;
+	      rtx dstv, srcv;
 
 	      vloc = loc;
 	      uloc = XEXP (vloc, 1);
 	      val = XEXP (vloc, 0);
 	      vloc = uloc;
 
+	      if (GET_CODE (uloc) == SET)
+		{
+		  dstv = SET_DEST (uloc);
+		  srcv = SET_SRC (uloc);
+		}
+	      else
+		{
+		  dstv = uloc;
+		  srcv = NULL;
+		}
+
 	      if (GET_CODE (val) == CONCAT)
 		{
-		  vloc = XEXP (val, 1);
+		  dstv = vloc = XEXP (val, 1);
 		  val = XEXP (val, 0);
 		}
 
 	      if (GET_CODE (vloc) == SET)
 		{
-		  rtx vsrc = SET_SRC (vloc);
+		  srcv = SET_SRC (vloc);
 
-		  gcc_assert (val != vsrc);
+		  gcc_assert (val != srcv);
 		  gcc_assert (vloc == uloc || VAL_NEEDS_RESOLUTION (loc));
 
-		  vloc = SET_DEST (vloc);
+		  dstv = vloc = SET_DEST (vloc);
 
 		  if (VAL_NEEDS_RESOLUTION (loc))
-		    val_resolve (set, val, vsrc, insn);
+		    val_resolve (set, val, srcv, insn);
 		}
 	      else if (VAL_NEEDS_RESOLUTION (loc))
 		{
@@ -8767,39 +8842,47 @@  emit_notes_in_bb (basic_block bb, datafl
 		      if (REG_P (uloc))
 			var_reg_delete (set, uloc, true);
 		      else if (MEM_P (uloc))
-			var_mem_delete (set, uloc, true);
+			{
+			  gcc_assert (MEM_P (dstv));
+			  gcc_assert (MEM_ATTRS (dstv) == MEM_ATTRS (uloc));
+			  var_mem_delete (set, dstv, true);
+			}
 		    }
 		  else
 		    {
 		      bool copied_p = VAL_EXPR_IS_COPIED (loc);
-		      rtx set_src = NULL;
+		      rtx src = NULL, dst = uloc;
 		      enum var_init_status status = VAR_INIT_STATUS_INITIALIZED;
 
 		      if (GET_CODE (uloc) == SET)
 			{
-			  set_src = SET_SRC (uloc);
-			  uloc = SET_DEST (uloc);
+			  src = SET_SRC (uloc);
+			  dst = SET_DEST (uloc);
 			}
 
 		      if (copied_p)
 			{
-			  status = find_src_status (set, set_src);
+			  status = find_src_status (set, src);
 
-			  set_src = find_src_set_src (set, set_src);
+			  src = find_src_set_src (set, src);
 			}
 
-		      if (REG_P (uloc))
-			var_reg_delete_and_set (set, uloc, !copied_p,
-						status, set_src);
-		      else if (MEM_P (uloc))
-			var_mem_delete_and_set (set, uloc, !copied_p,
-						status, set_src);
+		      if (REG_P (dst))
+			var_reg_delete_and_set (set, dst, !copied_p,
+						status, srcv);
+		      else if (MEM_P (dst))
+			{
+			  gcc_assert (MEM_P (dstv));
+			  gcc_assert (MEM_ATTRS (dstv) == MEM_ATTRS (dst));
+			  var_mem_delete_and_set (set, dstv, !copied_p,
+						  status, srcv);
+			}
 		    }
 		}
 	      else if (REG_P (uloc))
 		var_regno_delete (set, REGNO (uloc));
 
-	      val_store (set, val, vloc, insn, true);
+	      val_store (set, val, dstv, insn, true);
 
 	      emit_notes_for_changes (next_insn, EMIT_NOTE_BEFORE_INSN,
 				      set->vars);
@@ -8908,9 +8991,13 @@  vt_emit_notes (void)
   emit_notes = true;
 
   if (MAY_HAVE_DEBUG_INSNS)
-    dropped_values = htab_create (cselib_get_next_uid () * 2,
-				  variable_htab_hash, variable_htab_eq,
-				  variable_htab_free);
+    {
+      dropped_values = htab_create (cselib_get_next_uid () * 2,
+				    variable_htab_hash, variable_htab_eq,
+				    variable_htab_free);
+      loc_exp_dep_pool = create_alloc_pool ("loc_exp_dep pool",
+					    sizeof (loc_exp_dep), 64);
+    }
 
   dataflow_set_init (&cur);
 
@@ -8935,7 +9022,11 @@  vt_emit_notes (void)
   dataflow_set_destroy (&cur);
 
   if (MAY_HAVE_DEBUG_INSNS)
-    htab_delete (dropped_values);
+    {
+      free_alloc_pool (loc_exp_dep_pool);
+      loc_exp_dep_pool = NULL;
+      htab_delete (dropped_values);
+    }
 
   emit_notes = false;
 }