Patchwork [PR49888,VTA] don't keep VALUEs bound to modified MEMs

login
register
mail settings
Submitter Alexandre Oliva
Date May 23, 2012, 9:27 a.m.
Message ID <orhav7nt6u.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/160904/
State New
Headers show

Comments

Alexandre Oliva - May 23, 2012, 9:27 a.m.
Nothing in var-tracking was prepared to drop MEMs from bindings upon
writes that might modify the MEM contents.  That was correct, back when
it was variables (rather than values) that were bound to MEMs, and we
wanted the binding to remain even if the value stored in the variables
changed.

VTA changed that for gimple regs: we now bind variables to values, and
values to locations that hold that value, so we must unbind them when
the value stored in the MEM location changes, just like we already did
for REGs.

cselib might offer us some help to that end, but only within individual
basic blocks.  That's not quite enough: even a trivial testcase, as
provided in the bug report, was enough to defeat the first approach I
tried, of emitting MOps to unbind values from MEMs when cselib noticed
the MEM was modified.  Even in that trivial testcase, we wouldn't always
unbind the incoming argument from the MEM, because it would be recorded
using a different expression which we could only globally recognize as
equivalent.  And that didn't even take potential aliasing into account!

In order to fix this debug info correctness bug, I ended up going with
the following approach: after every MEM write, we go through all MEMs in
the dataflow set binding table and unbind VALUEs bound to
possibly-aliased MEMs.  I'm sure there are cases in which this may
remove perfectly valid location information, but getting the compiler to
figure out they are indeed valid is impossible in some cases (eg
incoming pointers known by the caller to not alias), and very, very
expensive in others (eg inferring the lack of overlap by following
equivalence chains).

In the end, this patch didn't cause much loss of debug information on
x86_64, but it does significantly reduce total coverage on i686 for
libstdc++, libgcc and cc1plus.  There was a also a significant drop in
call_site_value and entry_value expressions, on both x86_64 and i686,
slightly more pronounced on the latter.  Unfortunately, I couldn't
quantify how much of this reduction was because we of incorrect location
information we generated before, and how much is because of
overconservative decisions about potential aliasing.

This was all kind of expected.  What did surprise me was that this
didn't have any measurable impact on bootstrap time.  What surprised me
further was that testing this patch confirmed the boostrap speedup of
1.08 I had observed with the proposed patch for PR47624, that this patch
depends on.

This patch was regstrapped along with the patch for PR47624 (ping
<http://gcc.gnu.org/ml/gcc-patches/2012-04/msg01320.html>) on
x86_64-linux-gnu and i686-linux-gnu.  Ok to install?
Jakub Jelinek - May 23, 2012, 10:13 a.m.
On Wed, May 23, 2012 at 06:27:21AM -0300, Alexandre Oliva wrote:
> +static int
> +drop_overlapping_mem_locs (void **slot, void *data)
> +{
> +  struct overlapping_mems *coms = (struct overlapping_mems *)data;
> +  dataflow_set *set = coms->set;
> +  rtx mloc = coms->loc;
> +  variable var = (variable) *slot;
> +
> +  if (var->onepart == ONEPART_VALUE)
> +    {
> +      location_chain loc, *locp;
> +      bool changed = false;
> +      rtx cur_loc;
> +
> +      gcc_assert (var->n_var_parts == 1);
> +
> +      if (shared_var_p (var, set->vars))
> +	{
> +	  for (loc = var->var_part[0].loc_chain; loc; loc = loc->next)
> +	    if (GET_CODE (loc->loc) == MEM
> +		&& !nonoverlapping_memrefs_p (loc->loc, mloc, false))

Isn't nonoverlapping_memrefs_p predicate too conservative?
cselib.c uses canon_true_dependence to decide what should be invalidated.

	Jakub
Richard Guenther - May 23, 2012, 10:25 a.m.
On Wed, May 23, 2012 at 12:13 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, May 23, 2012 at 06:27:21AM -0300, Alexandre Oliva wrote:
>> +static int
>> +drop_overlapping_mem_locs (void **slot, void *data)
>> +{
>> +  struct overlapping_mems *coms = (struct overlapping_mems *)data;
>> +  dataflow_set *set = coms->set;
>> +  rtx mloc = coms->loc;
>> +  variable var = (variable) *slot;
>> +
>> +  if (var->onepart == ONEPART_VALUE)
>> +    {
>> +      location_chain loc, *locp;
>> +      bool changed = false;
>> +      rtx cur_loc;
>> +
>> +      gcc_assert (var->n_var_parts == 1);
>> +
>> +      if (shared_var_p (var, set->vars))
>> +     {
>> +       for (loc = var->var_part[0].loc_chain; loc; loc = loc->next)
>> +         if (GET_CODE (loc->loc) == MEM
>> +             && !nonoverlapping_memrefs_p (loc->loc, mloc, false))
>
> Isn't nonoverlapping_memrefs_p predicate too conservative?
> cselib.c uses canon_true_dependence to decide what should be invalidated.

Yeah, I think nonoverlapping_memrefs_p should become private to alias.c

Richard.

>        Jakub

Patch

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

	PR debug/49888
	* var-tracking.c: Include alias.h.
	(overlapping_mems): New struct.
	(drop_overlapping_mem_locs): New.
	(clobber_overlapping_mems): New.
	(var_mem_delete_and_set, var_mem_delete): Call it.
	(val_bind): Likewise, but only if modified.
	(compute_bb_dataflow, emit_notes_in_bb): Call it on MEMs.
	* Makefile.in (var-tracking.o): Depend in $(ALIAS_H).
	
for  gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/49888
	* gcc.dg/guality/pr49888.c: New.

Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c.orig	2012-05-16 14:58:11.000000000 -0300
+++ gcc/var-tracking.c	2012-05-21 02:24:57.000000000 -0300
@@ -116,6 +116,7 @@ 
 #include "pointer-set.h"
 #include "recog.h"
 #include "tm_p.h"
+#include "alias.h"
 
 /* var-tracking.c assumes that tree code with the same value as VALUE rtx code
    has no chance to appear in REG_EXPR/MEM_EXPRs and isn't a decl.
@@ -1955,6 +1956,106 @@  var_regno_delete (dataflow_set *set, int
   *reg = NULL;
 }
 
+/* Hold parameters for the hashtab traversal function
+   drop_overlapping_mem_locs, see below.  */
+
+struct overlapping_mems
+{
+  dataflow_set *set;
+  rtx loc;
+};
+
+/* Remove all MEMs that overlap with COMS->LOC from the location list
+   of a hash table entry for a value.  */
+
+static int
+drop_overlapping_mem_locs (void **slot, void *data)
+{
+  struct overlapping_mems *coms = (struct overlapping_mems *)data;
+  dataflow_set *set = coms->set;
+  rtx mloc = coms->loc;
+  variable var = (variable) *slot;
+
+  if (var->onepart == ONEPART_VALUE)
+    {
+      location_chain loc, *locp;
+      bool changed = false;
+      rtx cur_loc;
+
+      gcc_assert (var->n_var_parts == 1);
+
+      if (shared_var_p (var, set->vars))
+	{
+	  for (loc = var->var_part[0].loc_chain; loc; loc = loc->next)
+	    if (GET_CODE (loc->loc) == MEM
+		&& !nonoverlapping_memrefs_p (loc->loc, mloc, false))
+	      break;
+
+	  if (!loc)
+	    return 1;
+
+	  slot = unshare_variable (set, slot, var, VAR_INIT_STATUS_UNKNOWN);
+	  var = (variable)*slot;
+	  gcc_assert (var->n_var_parts == 1);
+	}
+
+      if (VAR_LOC_1PAUX (var))
+	cur_loc = VAR_LOC_FROM (var);
+      else
+	cur_loc = var->var_part[0].cur_loc;
+
+      for (locp = &var->var_part[0].loc_chain, loc = *locp;
+	   loc; loc = *locp)
+	{
+	  if (GET_CODE (loc->loc) != MEM
+	      || nonoverlapping_memrefs_p (loc->loc, mloc, false))
+	    {
+	      locp = &loc->next;
+	      continue;
+	    }
+
+	  *locp = loc->next;
+	  /* If we have deleted the location which was last emitted
+	     we have to emit new location so add the variable to set
+	     of changed variables.  */
+	  if (cur_loc == loc->loc)
+	    {
+	      changed = true;
+	      var->var_part[0].cur_loc = NULL;
+	      if (VAR_LOC_1PAUX (var))
+		VAR_LOC_FROM (var) = NULL;
+	    }
+	  pool_free (loc_chain_pool, loc);
+	}
+
+      if (!var->var_part[0].loc_chain)
+	{
+	  var->n_var_parts--;
+	  changed = true;
+	}
+      if (changed)
+	variable_was_changed (var, set);
+    }
+
+  return 1;
+}
+
+/* Remove from SET all VALUE bindings to MEMs that overlap with LOC.  */
+
+static void
+clobber_overlapping_mems (dataflow_set *set, rtx loc)
+{
+  struct overlapping_mems coms;
+
+  coms.set = set;
+  coms.loc = loc;
+
+  set->traversed_vars = set->vars;
+  htab_traverse (shared_hash_htab (set->vars),
+		 drop_overlapping_mem_locs, &coms);
+  set->traversed_vars = NULL;
+}
+
 /* Set the location of DV, OFFSET as the MEM LOC.  */
 
 static void
@@ -1997,6 +2098,7 @@  var_mem_delete_and_set (dataflow_set *se
   tree decl = MEM_EXPR (loc);
   HOST_WIDE_INT offset = INT_MEM_OFFSET (loc);
 
+  clobber_overlapping_mems (set, loc);
   decl = var_debug_decl (decl);
 
   if (initialized == VAR_INIT_STATUS_UNKNOWN)
@@ -2017,6 +2119,7 @@  var_mem_delete (dataflow_set *set, rtx l
   tree decl = MEM_EXPR (loc);
   HOST_WIDE_INT offset = INT_MEM_OFFSET (loc);
 
+  clobber_overlapping_mems (set, loc);
   decl = var_debug_decl (decl);
   if (clobber)
     clobber_variable_part (set, NULL, dv_from_decl (decl), offset, NULL);
@@ -2060,6 +2163,9 @@  val_bind (dataflow_set *set, rtx val, rt
     {
       struct elt_loc_list *l = CSELIB_VAL_PTR (val)->locs;
 
+      if (modified)
+	clobber_overlapping_mems (set, loc);
+
       if (l && GET_CODE (l->loc) == VALUE)
 	l = canonical_cselib_val (CSELIB_VAL_PTR (l->loc))->locs;
 
@@ -6372,6 +6478,8 @@  compute_bb_dataflow (basic_block bb)
 		}
 	      else if (REG_P (uloc))
 		var_regno_delete (out, REGNO (uloc));
+	      else if (MEM_P (uloc))
+		clobber_overlapping_mems (out, uloc);
 
 	      val_store (out, val, dstv, insn, true);
 	    }
@@ -8871,6 +8979,8 @@  emit_notes_in_bb (basic_block bb, datafl
 		}
 	      else if (REG_P (uloc))
 		var_regno_delete (set, REGNO (uloc));
+	      else if (MEM_P (uloc))
+		clobber_overlapping_mems (set, uloc);
 
 	      val_store (set, val, dstv, insn, true);
 
Index: gcc/Makefile.in
===================================================================
--- gcc/Makefile.in.orig	2012-05-21 02:21:12.424703696 -0300
+++ gcc/Makefile.in	2012-05-21 02:22:26.000000000 -0300
@@ -3129,8 +3129,8 @@  var-tracking.o : var-tracking.c $(CONFIG
    $(RTL_H) $(TREE_H) hard-reg-set.h insn-config.h reload.h $(FLAGS_H) \
    $(BASIC_BLOCK_H) output.h sbitmap.h alloc-pool.h $(FIBHEAP_H) $(HASHTAB_H) \
    $(REGS_H) $(EXPR_H) $(TIMEVAR_H) $(TREE_PASS_H) $(TREE_FLOW_H) \
-   cselib.h $(TARGET_H) $(DIAGNOSTIC_CORE_H) $(PARAMS_H) $(DIAGNOSTIC_H) pointer-set.h \
-   $(RECOG_H) $(TM_P_H) tree-pretty-print.h
+   cselib.h $(TARGET_H) $(DIAGNOSTIC_CORE_H) $(PARAMS_H) $(DIAGNOSTIC_H) \
+   pointer-set.h $(RECOG_H) $(TM_P_H) tree-pretty-print.h $(ALIAS_H)
 profile.o : profile.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(RTL_H) \
    $(TREE_H) $(FLAGS_H) output.h $(REGS_H) $(EXPR_H) $(FUNCTION_H) $(BASIC_BLOCK_H) \
    $(DIAGNOSTIC_CORE_H) $(COVERAGE_H) $(TREE_FLOW_H) value-prof.h cfghooks.h \
Index: gcc/testsuite/gcc.dg/guality/pr49888.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.dg/guality/pr49888.c	2012-05-16 14:58:12.000000000 -0300
@@ -0,0 +1,25 @@ 
+/* PR debug/49888 */
+/* { dg-do run } */
+/* { dg-options "-g" } */
+
+static int v;
+
+static void __attribute__((noinline, noclone))
+f (int *p)
+{
+  int c = *p;
+  v = c;
+  *p = 1; /* { dg-final { gdb-test 12 "c" "0" } } */
+  /* c may very well be optimized out at this point, so we test !c,
+     which will evaluate to the expected value.  We just want to make
+     sure it doesn't remain bound to *p as it did before, in which
+     case !c would evaluate to 0.  */
+  v = 0; /* { dg-final { gdb-test 17 "!c" "1" } } */
+}
+int
+main ()
+{
+  int a = 0;
+  f (&a);
+  return 0;
+}