diff mbox

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

Message ID orvd0l6u7u.fsf@livre.localdomain
State New
Headers show

Commit Message

Alexandre Oliva Feb. 15, 2011, 6:14 p.m. UTC 
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?

Comments

Jakub Jelinek Feb. 15, 2011, 7:07 p.m. UTC | #1 
On Tue, Feb 15, 2011 at 04:14:29PM -0200, Alexandre Oliva 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?
> 

While I think it is a good idea, I'd say this should be stage 1 material,
after all it isn't a regression.  For the testcase in question I think
we also want to improve cselib.c as I wrote in the PR, to canonicalize
locs of VALUE + CONST_INT M when VALUE has locs of the form VALUE' + CONST_INT N
to VALUE' + CONST_INT (M + N), then we'd figure out we should use fbreg
and don't need to change the location description through the function.

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

s/deug/debug/

> 	* var-tracking.c (add_stores): Add FIXME note.
> 	(compute_bb_dataflow): Use value-based locations in MO_VAL_SET.
> 	(emit_notes_in_bb): Likewise.
> 	(check_changed_vars_4): New.
> 	(emit_noets_for_changes): Call it.

s/noets/notes/

	Jakub
Alexandre Oliva June 4, 2011, 10:43 a.m. UTC | #2 
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

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

> 	PR deug/47624
> 	* var-tracking.c (add_stores): Add FIXME note.
> 	(compute_bb_dataflow): Use value-based locations in MO_VAL_SET.
> 	(emit_notes_in_bb): Likewise.
> 	(check_changed_vars_4): New.
> 	(emit_noets_for_changes): Call it.
diff mbox

Patch

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

	PR deug/47624
	* var-tracking.c (add_stores): Add FIXME note.
	(compute_bb_dataflow): Use value-based locations in MO_VAL_SET.
	(emit_notes_in_bb): Likewise.
	(check_changed_vars_4): New.
	(emit_noets_for_changes): Call it.

Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c.orig	2011-02-14 16:53:32.296755325 -0200
+++ gcc/var-tracking.c	2011-02-14 18:35:17.011516560 -0200
@@ -5468,6 +5468,12 @@  add_stores (rtx loc, const_rtx expr, voi
 	    resolve = false;
 	}
     }
+  /* ??? This could make our dataflow sets independent from cselib
+     internal state, possibly simplifying the value expansion code and
+     removing inconsistencies when the only available location is the
+     one given by the initial definition.  */
+  else if (0 && resolve && GET_CODE (oloc) != SET && v->locs)
+    oloc = gen_rtx_SET (VOIDmode, oloc, v->locs->loc);
   else if (!resolve)
     {
       if (GET_CODE (mo.u.loc) == SET
@@ -5828,6 +5834,7 @@  compute_bb_dataflow (basic_block bb)
 	    {
 	      rtx loc = mo->u.loc;
 	      rtx val, vloc, uloc, reverse = NULL_RTX;
+	      rtx dstv, srcv;
 
 	      vloc = loc;
 	      if (VAL_EXPR_HAS_REVERSE (loc))
@@ -5839,23 +5846,34 @@  compute_bb_dataflow (basic_block bb)
 	      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))
 		{
@@ -5871,45 +5889,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);
 
 	      if (reverse)
 		val_store (out, XEXP (reverse, 0), XEXP (reverse, 1),
@@ -7457,6 +7483,33 @@  check_changed_vars_3 (void **slot, void 
   return 1;
 }
 
+/* If there is a multi-part variable in SLOT, check whether the
+   location of any of its parts is given by VALUEs that changed.
+
+   ??? Maybe add multi-part variables to value chains, to avoid having
+   to go through all variables in the table.  */
+
+static int
+check_changed_vars_4 (void **slot, void *data)
+{
+  variable var = (variable) *slot;
+  htab_t vars = (htab_t) data;
+
+  if (dv_changed_p (var->dv) || dv_onepart_p (var->dv))
+    return 1;
+
+  if (!var->cur_loc_changed)
+    check_changed_vars_3 (slot, vars);
+
+  if (var->cur_loc_changed)
+    {
+      set_dv_changed (var->dv, true);
+      variable_was_changed (var, NULL);
+    }
+
+  return 1;
+}
+
 /* Emit NOTE_INSN_VAR_LOCATION note for each variable from a chain
    CHANGED_VARIABLES and delete this chain.  WHERE specifies whether the notes
    shall be emitted before of after instruction INSN.  */
@@ -7484,6 +7537,7 @@  emit_notes_for_changes (rtx insn, enum e
 	set_dv_changed (dv_from_value (VEC_pop (rtx, changed_values_stack)),
 			false);
       htab_traverse (changed_variables, check_changed_vars_3, htab);
+      htab_traverse (htab, check_changed_vars_4, htab);
     }
 
   data.insn = insn;
@@ -7746,6 +7800,7 @@  emit_notes_in_bb (basic_block bb, datafl
 	    {
 	      rtx loc = mo->u.loc;
 	      rtx val, vloc, uloc, reverse = NULL_RTX;
+	      rtx dstv, srcv;
 
 	      vloc = loc;
 	      if (VAL_EXPR_HAS_REVERSE (loc))
@@ -7757,23 +7812,34 @@  emit_notes_in_bb (basic_block bb, datafl
 	      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))
 		{
@@ -7789,39 +7855,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);
 
 	      if (reverse)
 		val_store (set, XEXP (reverse, 0), XEXP (reverse, 1),