cselib: Don't consider SP_DERIVED_VALUE_P values as useless [PR94468]

Hi!

The following testcase ICEs, because at one point we see the
SP_DERIVED_VALUE_P VALUE as useless (not PRESERVED_VALUE_P and no locs)
and so expect it to be discarded as useless.  But, later on we
are adding some new VALUE that is equivalent to it, and when adding
the equivalency that that new VALUE is equal to this SP_DERIVED_VALUE_P,
new_elt_loc_list has code for VALUE canonicalization and reverses addition
if uid is smaller, and at that point a new loc is added to the
SP_DERIVED_VALUE_P VALUE and it isn't discarded as useless anymore.
Now, I think we don't want to discard the SP_DERIVED_VALUE_P values
even if they have no locs, because they still have the special behaviour
that they then force other new VALUEs to be canonicalized against them,
which is what this patch implements.  I've not set PRESERVED_VALUE_P
on the SP_DERIVED_VALUE_P at the creation time, because whether a VALUE
is preserved or not is something that affects var-tracking decisions quite a
lot and we shouldn't set it blindly on other VALUEs.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Or, to avoid the repetitive code, should I introduce
static bool
cselib_useless_value_p (cselib_val *v)
{
  return (v->locs == 0
	  && !PRESERVED_VALUE_P (v->val_rtx)
	  && !SP_DERIVED_VALUE_P (v->val_rtx)));
}
predicate and use it in those 6 spots?

2020-04-03  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/94468
	* cselib.c (references_value_p): Formatting fix.
	(discard_useless_locs, discard_useless_values,
	cselib_invalidate_regno_val, cselib_invalidate_mem,
	cselib_record_set): Don't consider SP_DERIVED_VALUE_P values useless.

	* g++.dg/opt/pr94468.C: New test.


	Jakub

On Fri, Apr 03, 2020 at 07:42:10PM +0200, Jakub Jelinek via Gcc-patches wrote:
> Or, to avoid the repetitive code, should I introduce
> static bool
> cselib_useless_value_p (cselib_val *v)
> {
>   return (v->locs == 0
> 	  && !PRESERVED_VALUE_P (v->val_rtx)
> 	  && !SP_DERIVED_VALUE_P (v->val_rtx)));
> }
> predicate and use it in those 6 spots?

Here is the patch variant with the above helper.
Also bootstrapped/regtested on x86_64-linux and i686-linux.

2020-04-04  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/94468
	* cselib.c (references_value_p): Formatting fix.
	(cselib_useless_value_p): New function.
	(discard_useless_locs, discard_useless_values,
	cselib_invalidate_regno_val, cselib_invalidate_mem,
	cselib_record_set): Use it instead of
	v->locs == 0 && !PRESERVED_VALUE_P (v->val_rtx).

	* g++.dg/opt/pr94468.C: New test.

--- gcc/cselib.c.jj	2020-04-02 14:28:02.620577679 +0200
+++ gcc/cselib.c	2020-04-03 17:08:54.295282018 +0200
@@ -629,8 +629,8 @@ references_value_p (const_rtx x, int onl
   int i, j;
 
   if (GET_CODE (x) == VALUE
-      && (! only_useless ||
-	  (CSELIB_VAL_PTR (x)->locs == 0 && !PRESERVED_VALUE_P (x))))
+      && (! only_useless
+	  || (CSELIB_VAL_PTR (x)->locs == 0 && !PRESERVED_VALUE_P (x))))
     return 1;
 
   for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
@@ -646,6 +646,16 @@ references_value_p (const_rtx x, int onl
   return 0;
 }
 
+/* Return true if V is a useless VALUE and can be discarded as such.  */
+
+static bool
+cselib_useless_value_p (cselib_val *v)
+{
+  return (v->locs == 0
+	  && !PRESERVED_VALUE_P (v->val_rtx)
+	  && !SP_DERIVED_VALUE_P (v->val_rtx));
+}
+
 /* For all locations found in X, delete locations that reference useless
    values (i.e. values without any location).  Called through
    htab_traverse.  */
@@ -666,7 +676,7 @@ discard_useless_locs (cselib_val **x, vo
 	p = &(*p)->next;
     }
 
-  if (had_locs && v->locs == 0 && !PRESERVED_VALUE_P (v->val_rtx))
+  if (had_locs && cselib_useless_value_p (v))
     {
       if (setting_insn && DEBUG_INSN_P (setting_insn))
 	n_useless_debug_values++;
@@ -684,7 +694,7 @@ discard_useless_values (cselib_val **x,
 {
   cselib_val *v = *x;
 
-  if (v->locs == 0 && !PRESERVED_VALUE_P (v->val_rtx))
+  if (v->locs == 0 && cselib_useless_value_p (v))
     {
       if (cselib_discard_hook)
 	cselib_discard_hook (v);
@@ -2370,7 +2380,7 @@ cselib_invalidate_regno_val (unsigned in
 	}
     }
 
-  if (had_locs && v->locs == 0 && !PRESERVED_VALUE_P (v->val_rtx))
+  if (had_locs && cselib_useless_value_p (v))
     {
       if (setting_insn && DEBUG_INSN_P (setting_insn))
 	n_useless_debug_values++;
@@ -2515,7 +2525,7 @@ cselib_invalidate_mem (rtx mem_rtx)
 	  unchain_one_elt_loc_list (p);
 	}
 
-      if (had_locs && v->locs == 0 && !PRESERVED_VALUE_P (v->val_rtx))
+      if (had_locs && cselib_useless_value_p (v))
 	{
 	  if (setting_insn && DEBUG_INSN_P (setting_insn))
 	    n_useless_debug_values++;
@@ -2593,14 +2603,14 @@ cselib_record_set (rtx dest, cselib_val
 	  REG_VALUES (dreg)->elt = src_elt;
 	}
 
-      if (src_elt->locs == 0 && !PRESERVED_VALUE_P (src_elt->val_rtx))
+      if (cselib_useless_value_p (src_elt))
 	n_useless_values--;
       new_elt_loc_list (src_elt, dest);
     }
   else if (MEM_P (dest) && dest_addr_elt != 0
 	   && cselib_record_memory)
     {
-      if (src_elt->locs == 0 && !PRESERVED_VALUE_P (src_elt->val_rtx))
+      if (cselib_useless_value_p (src_elt))
 	n_useless_values--;
       add_mem_for_addr (dest_addr_elt, src_elt, dest);
     }
--- gcc/testsuite/g++.dg/opt/pr94468.C.jj	2020-04-03 17:16:38.804457422 +0200
+++ gcc/testsuite/g++.dg/opt/pr94468.C	2020-04-03 17:16:18.450756522 +0200
@@ -0,0 +1,57 @@
+// PR rtl-optimization/94468
+// { dg-do compile { target c++11 } }
+// { dg-options "-O2" }
+// { dg-additional-options "-fPIC" { target fpic } }
+
+bool a();
+enum b {};
+class c;
+template <typename> struct d;
+template <class e, typename g, typename... h> struct d<g (e::*)(h...)> {
+  typedef e i;
+};
+struct j { j(void(int, j *, c *, void **, bool *)) {} };
+template <typename l> struct m : public j {
+  l ab;
+  static void ac(int, j *, c *, void **, bool *);
+  m(l f) : j(ac), ab(f) {}
+};
+b ad;
+struct c {
+  template <typename n, typename o>
+  void ae(typename d<n>::i *p, n af, typename d<o>::i *ag, o ah) {
+    ai(p, &af, ag, &ah, new m<o>(ah), ad, &d<n>::i::aj);
+  }
+  void ai(c *, void *, c *, void *, j *, b, int *);
+};
+struct r : public c { static int aj; void t(); };
+struct al : public c {
+  static int aj;
+  void am();
+  void ao();
+  void ap();
+};
+struct aq { aq(const int &, const int & = int()); };
+struct ar : public c { ~ar(); };
+struct as : public ar {
+  as();
+  void at();
+  void au();
+  void av();
+};
+struct u : public c { void ax(); };
+struct ay { int az(); };
+struct ba : public c { static int aj; void bb(); };
+struct bc : public al { bc() { if (a()) am(); } };
+as::as() {
+  al *bd = new bc;
+  ae(bd, &al::ao, this, &as::au);
+  ae(bd, &al::ap, this, &as::av);
+  r be;
+  u bf;
+  ae(&be, &r::t, &bf, &u::ax);
+  c bg = *bd;
+  ae(static_cast<ba *>(&bg), &ba::bb, this, &as::at);
+  ay bh;
+  aq am(bh.az());
+}

	Jakub

On April 4, 2020 12:42:33 AM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Fri, Apr 03, 2020 at 07:42:10PM +0200, Jakub Jelinek via Gcc-patches
>wrote:
>> Or, to avoid the repetitive code, should I introduce
>> static bool
>> cselib_useless_value_p (cselib_val *v)
>> {
>>   return (v->locs == 0
>> 	  && !PRESERVED_VALUE_P (v->val_rtx)
>> 	  && !SP_DERIVED_VALUE_P (v->val_rtx)));
>> }
>> predicate and use it in those 6 spots?
>
>Here is the patch variant with the above helper.
>Also bootstrapped/regtested on x86_64-linux and i686-linux.

OK. 

Richard. 

>2020-04-04  Jakub Jelinek  <jakub@redhat.com>
>
>	PR rtl-optimization/94468
>	* cselib.c (references_value_p): Formatting fix.
>	(cselib_useless_value_p): New function.
>	(discard_useless_locs, discard_useless_values,
>	cselib_invalidate_regno_val, cselib_invalidate_mem,
>	cselib_record_set): Use it instead of
>	v->locs == 0 && !PRESERVED_VALUE_P (v->val_rtx).
>
>	* g++.dg/opt/pr94468.C: New test.
>
>--- gcc/cselib.c.jj	2020-04-02 14:28:02.620577679 +0200
>+++ gcc/cselib.c	2020-04-03 17:08:54.295282018 +0200
>@@ -629,8 +629,8 @@ references_value_p (const_rtx x, int onl
>   int i, j;
> 
>   if (GET_CODE (x) == VALUE
>-      && (! only_useless ||
>-	  (CSELIB_VAL_PTR (x)->locs == 0 && !PRESERVED_VALUE_P (x))))
>+      && (! only_useless
>+	  || (CSELIB_VAL_PTR (x)->locs == 0 && !PRESERVED_VALUE_P (x))))
>     return 1;
> 
>   for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
>@@ -646,6 +646,16 @@ references_value_p (const_rtx x, int onl
>   return 0;
> }
> 
>+/* Return true if V is a useless VALUE and can be discarded as such. 
>*/
>+
>+static bool
>+cselib_useless_value_p (cselib_val *v)
>+{
>+  return (v->locs == 0
>+	  && !PRESERVED_VALUE_P (v->val_rtx)
>+	  && !SP_DERIVED_VALUE_P (v->val_rtx));
>+}
>+
>/* For all locations found in X, delete locations that reference
>useless
>    values (i.e. values without any location).  Called through
>    htab_traverse.  */
>@@ -666,7 +676,7 @@ discard_useless_locs (cselib_val **x, vo
> 	p = &(*p)->next;
>     }
> 
>-  if (had_locs && v->locs == 0 && !PRESERVED_VALUE_P (v->val_rtx))
>+  if (had_locs && cselib_useless_value_p (v))
>     {
>       if (setting_insn && DEBUG_INSN_P (setting_insn))
> 	n_useless_debug_values++;
>@@ -684,7 +694,7 @@ discard_useless_values (cselib_val **x,
> {
>   cselib_val *v = *x;
> 
>-  if (v->locs == 0 && !PRESERVED_VALUE_P (v->val_rtx))
>+  if (v->locs == 0 && cselib_useless_value_p (v))
>     {
>       if (cselib_discard_hook)
> 	cselib_discard_hook (v);
>@@ -2370,7 +2380,7 @@ cselib_invalidate_regno_val (unsigned in
> 	}
>     }
> 
>-  if (had_locs && v->locs == 0 && !PRESERVED_VALUE_P (v->val_rtx))
>+  if (had_locs && cselib_useless_value_p (v))
>     {
>       if (setting_insn && DEBUG_INSN_P (setting_insn))
> 	n_useless_debug_values++;
>@@ -2515,7 +2525,7 @@ cselib_invalidate_mem (rtx mem_rtx)
> 	  unchain_one_elt_loc_list (p);
> 	}
> 
>-      if (had_locs && v->locs == 0 && !PRESERVED_VALUE_P (v->val_rtx))
>+      if (had_locs && cselib_useless_value_p (v))
> 	{
> 	  if (setting_insn && DEBUG_INSN_P (setting_insn))
> 	    n_useless_debug_values++;
>@@ -2593,14 +2603,14 @@ cselib_record_set (rtx dest, cselib_val
> 	  REG_VALUES (dreg)->elt = src_elt;
> 	}
> 
>-      if (src_elt->locs == 0 && !PRESERVED_VALUE_P (src_elt->val_rtx))
>+      if (cselib_useless_value_p (src_elt))
> 	n_useless_values--;
>       new_elt_loc_list (src_elt, dest);
>     }
>   else if (MEM_P (dest) && dest_addr_elt != 0
> 	   && cselib_record_memory)
>     {
>-      if (src_elt->locs == 0 && !PRESERVED_VALUE_P (src_elt->val_rtx))
>+      if (cselib_useless_value_p (src_elt))
> 	n_useless_values--;
>       add_mem_for_addr (dest_addr_elt, src_elt, dest);
>     }
>--- gcc/testsuite/g++.dg/opt/pr94468.C.jj	2020-04-03 17:16:38.804457422
>+0200
>+++ gcc/testsuite/g++.dg/opt/pr94468.C	2020-04-03 17:16:18.450756522
>+0200
>@@ -0,0 +1,57 @@
>+// PR rtl-optimization/94468
>+// { dg-do compile { target c++11 } }
>+// { dg-options "-O2" }
>+// { dg-additional-options "-fPIC" { target fpic } }
>+
>+bool a();
>+enum b {};
>+class c;
>+template <typename> struct d;
>+template <class e, typename g, typename... h> struct d<g (e::*)(h...)>
>{
>+  typedef e i;
>+};
>+struct j { j(void(int, j *, c *, void **, bool *)) {} };
>+template <typename l> struct m : public j {
>+  l ab;
>+  static void ac(int, j *, c *, void **, bool *);
>+  m(l f) : j(ac), ab(f) {}
>+};
>+b ad;
>+struct c {
>+  template <typename n, typename o>
>+  void ae(typename d<n>::i *p, n af, typename d<o>::i *ag, o ah) {
>+    ai(p, &af, ag, &ah, new m<o>(ah), ad, &d<n>::i::aj);
>+  }
>+  void ai(c *, void *, c *, void *, j *, b, int *);
>+};
>+struct r : public c { static int aj; void t(); };
>+struct al : public c {
>+  static int aj;
>+  void am();
>+  void ao();
>+  void ap();
>+};
>+struct aq { aq(const int &, const int & = int()); };
>+struct ar : public c { ~ar(); };
>+struct as : public ar {
>+  as();
>+  void at();
>+  void au();
>+  void av();
>+};
>+struct u : public c { void ax(); };
>+struct ay { int az(); };
>+struct ba : public c { static int aj; void bb(); };
>+struct bc : public al { bc() { if (a()) am(); } };
>+as::as() {
>+  al *bd = new bc;
>+  ae(bd, &al::ao, this, &as::au);
>+  ae(bd, &al::ap, this, &as::av);
>+  r be;
>+  u bf;
>+  ae(&be, &r::t, &bf, &u::ax);
>+  c bg = *bd;
>+  ae(static_cast<ba *>(&bg), &ba::bb, this, &as::at);
>+  ay bh;
>+  aq am(bh.az());
>+}
>
>	Jakub