diff mbox

Don't add REG_EQUAL notes in fwprop for paradoxical subregs (PR rtl-optimization/70574)

Message ID 20160408170511.GY19207@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek April 8, 2016, 5:05 p.m. UTC 
On Fri, Apr 08, 2016 at 01:18:50PM +0200, Bernd Schmidt wrote:
> On 04/07/2016 11:56 PM, Jakub Jelinek wrote:
> >Not sure if this patch catches everything though, perhaps there could be
> >e.g.
> >(set (reg:SI ...) (plus:SI ((subreg:SI (reg:QI ...) 0) (const_int ...)))
> >and we'd still assign REG_EQUAL note.  So maybe instead we should walk the
> >*loc expression and look for paradoxical subregs, and for each of them, if
> >we find the DF_REF_REG (use) mentioned in their operand, clear
> >set_reg_equal.  Though of course, if DF_REF_REG (use) itself is a
> >paradoxical subreg, we could clear set_reg_equal without any walking.
> 
> It seems like something like that could happen.
> 
> How much do we lose if we just don't make new REG_EQUAL notes here?

After IRC discussions, I've bootstrapped/regtested following patch that
just punts if *loc contains any paradoxical subregs, together with
additional statistics gathering that proved that the new testcase is
the only spot in which this patch makes a difference on x86_64-linux
and i686-linux bootstrap/regtest.

Of course on other targets it might affect more.

Ok for trunk?

2016-04-08  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/70574
	* fwprop.c (forward_propagate_and_simplify): Don't add
	REG_EQUAL note if DF_REF_REG (use) is a paradoxical subreg.
	(try_fwprop_subst): Don't add REG_EQUAL note if there are any
	paradoxical subregs within *loc.

	* gcc.target/i386/avx2-pr70574.c: New test.



	Jakub

Comments

Bernd Schmidt April 8, 2016, 5:17 p.m. UTC | #1 
On 04/08/2016 07:05 PM, Jakub Jelinek wrote:
> After IRC discussions, I've bootstrapped/regtested following patch that
> just punts if *loc contains any paradoxical subregs, together with
> additional statistics gathering that proved that the new testcase is
> the only spot in which this patch makes a difference on x86_64-linux
> and i686-linux bootstrap/regtest.
>
> Of course on other targets it might affect more.

I ran the test with just not generating any REG_EQUAL notes here in 
fwprop, and out of 4492 source files only three showed code generation 
differences, which suggests we're not getting much value out of these.

Two of the cases where the same missed tree optimization, for which I've 
filed PR70600.

> 2016-04-08  Jakub Jelinek  <jakub@redhat.com>
>
> 	PR rtl-optimization/70574
> 	* fwprop.c (forward_propagate_and_simplify): Don't add
> 	REG_EQUAL note if DF_REF_REG (use) is a paradoxical subreg.
> 	(try_fwprop_subst): Don't add REG_EQUAL note if there are any
> 	paradoxical subregs within *loc.
>
> 	* gcc.target/i386/avx2-pr70574.c: New test.

Ok.


Bernd
diff mbox

Patch

--- gcc/fwprop.c.jj	2016-04-07 23:27:41.396310248 +0200
+++ gcc/fwprop.c	2016-04-08 15:51:09.164706722 +0200
@@ -999,10 +999,27 @@  try_fwprop_subst (df_ref use, rtx *loc,
 	 making a new one if one does not already exist.  */
       if (set_reg_equal)
 	{
-	  if (dump_file)
-	    fprintf (dump_file, " Setting REG_EQUAL note\n");
+	  /* If there are any paradoxical SUBREGs, don't add REG_EQUAL note,
+	     because the bits in there can be anything and so might not
+	     match the REG_EQUAL note content.  See PR70574.  */
+	  subrtx_var_iterator::array_type array;
+	  FOR_EACH_SUBRTX_VAR (iter, array, *loc, NONCONST)
+	    {
+	      rtx x = *iter;
+	      if (SUBREG_P (x) && paradoxical_subreg_p (x))
+		{
+		  set_reg_equal = false;
+		  break;
+		}
+	    }
+
+	  if (set_reg_equal)
+	    {
+	      if (dump_file)
+		fprintf (dump_file, " Setting REG_EQUAL note\n");
 
-	  note = set_unique_reg_note (insn, REG_EQUAL, copy_rtx (new_rtx));
+	      note = set_unique_reg_note (insn, REG_EQUAL, copy_rtx (new_rtx));
+	    }
 	}
     }
 
@@ -1300,14 +1317,19 @@  forward_propagate_and_simplify (df_ref u
 	 that isn't mentioned in USE_SET, as the note would be invalid
 	 otherwise.  We also don't want to install a note if we are merely
 	 propagating a pseudo since verifying that this pseudo isn't dead
-	 is a pain; moreover such a note won't help anything.  */
+	 is a pain; moreover such a note won't help anything.
+	 If the use is a paradoxical subreg, make sure we don't add a
+	 REG_EQUAL note for it, because it is not equivalent, it is one
+	 possible value for it, but we can't rely on it holding that value.
+	 See PR70574.  */
       set_reg_equal = (note == NULL_RTX
 		       && REG_P (SET_DEST (use_set))
 		       && !REG_P (src)
 		       && !(GET_CODE (src) == SUBREG
 			    && REG_P (SUBREG_REG (src)))
 		       && !reg_mentioned_p (SET_DEST (use_set),
-					    SET_SRC (use_set)));
+					    SET_SRC (use_set))
+		       && !paradoxical_subreg_p (DF_REF_REG (use)));
     }
 
   if (GET_MODE (*loc) == VOIDmode)
--- gcc/testsuite/gcc.target/i386/avx2-pr70574.c.jj	2016-04-08 13:32:04.525196849 +0200
+++ gcc/testsuite/gcc.target/i386/avx2-pr70574.c	2016-04-08 13:32:04.525196849 +0200
@@ -0,0 +1,26 @@ 
+/* PR rtl-optimization/70574 */
+/* { dg-do run { target lp64 } } */
+/* { dg-require-effective-target avx2 } */
+/* { dg-options "-O -frerun-cse-after-loop -fno-tree-ccp -mcmodel=medium -mavx2" } */
+/* { dg-additional-options "-fPIC" { target fpic } } */
+
+#include "avx2-check.h"
+
+typedef char A __attribute__((vector_size (32)));
+typedef short B __attribute__((vector_size (32)));
+
+int
+foo (int x, __int128 y, __int128 z, A w)
+{
+  y <<= 64;
+  w *= (A) { 0, -1, z, 0, ~y };
+  return w[0] + ((B) { x, 0, y, 0, -1 } | 1)[4];
+}
+
+static void
+avx2_test ()
+{
+  int x = foo (0, 0, 0, (A) {});
+  if (x != -1)
+    __builtin_abort ();
+}