combine: Update links correctly for new I2 (PR84169)

Message ID 8a65498338913b3902abaf7ac325df4ce230cd08.1518389741.git.segher@kernel.crashing.org
State New
Headers show
Series
  • combine: Update links correctly for new I2 (PR84169)
Related show

Commit Message

Segher Boessenkool Feb. 12, 2018, 3:59 p.m.
If there is a LOG_LINK between two insns, this means those two insns
can be combined, as far as dataflow is concerned.  There never should
be a LOG_LINK between two unrelated insns.  If there is one, combine
will try to combine the insns without doing all the needed checks if
the earlier destination is used before the later insn, etc.

Unfortunately we do not update the LOG_LINKs correctly in some cases.
This patch fixes at least some of those cases.

This fixes the PR's testcase on aarch64.  Also tested on 30+ cross
compiler, and on powerpc64-linux {-m32,-m64}.  Will test on x86_64
as well before committing.


Segher


2018-02-12  Segher Boessenkool  <segher@kernel.crashing.org>

	PR rtl-optimization/84169
	* combine.c (try_combine): New variable split_i2i3.  Set it to true if
	we generated a parallel as new i3 and we split that to new i2 and i3
	instructions.  Handle split_i2i3 similar to swap_i2i3: scan the
	LOG_LINKs of i3 to see which of those need to link to i2 now.  Link
	those to i2, not i1.  Partially rewrite this scan code.

---
 gcc/combine.c | 55 ++++++++++++++++++++++++++++++-------------------------
 1 file changed, 30 insertions(+), 25 deletions(-)

Comments

Jakub Jelinek Feb. 12, 2018, 4:12 p.m. | #1
On Mon, Feb 12, 2018 at 03:59:05PM +0000, Segher Boessenkool wrote:
> If there is a LOG_LINK between two insns, this means those two insns
> can be combined, as far as dataflow is concerned.  There never should
> be a LOG_LINK between two unrelated insns.  If there is one, combine
> will try to combine the insns without doing all the needed checks if
> the earlier destination is used before the later insn, etc.
> 
> Unfortunately we do not update the LOG_LINKs correctly in some cases.
> This patch fixes at least some of those cases.
> 
> This fixes the PR's testcase on aarch64.  Also tested on 30+ cross
> compiler, and on powerpc64-linux {-m32,-m64}.  Will test on x86_64
> as well before committing.

Will you check in the testcase too?

My preference would be something like following, so that it can
be torture-tested on all targets.

--- gcc/testsuite/gcc.c-torture/execute/pr84169.c	2018-01-12 10:39:42.940283691 +0100
+++ gcc/testsuite/gcc.c-torture/execute/pr84169.c	2018-02-12 17:11:18.970878978 +0100
@@ -0,0 +1,25 @@
+/* PR rtl-optimization/84169 */
+
+#ifdef __SIZEOF_INT128__
+typedef unsigned __int128 T;
+#else
+typedef unsigned long long T;
+#endif
+
+T b;
+
+static __attribute__ ((noipa)) T
+foo (T c, T d, T e, T f, T g, T h)
+{
+  __builtin_mul_overflow ((unsigned char) h, -16, &h);
+  return b + h;
+}
+
+int
+main ()
+{
+  T x = foo (0, 0, 0, 0, 0, 4);
+  if (x != -64)
+    __builtin_abort ();
+  return 0;
+}


	Jakub
Segher Boessenkool Feb. 12, 2018, 4:36 p.m. | #2
On Mon, Feb 12, 2018 at 05:12:20PM +0100, Jakub Jelinek wrote:
> On Mon, Feb 12, 2018 at 03:59:05PM +0000, Segher Boessenkool wrote:
> > If there is a LOG_LINK between two insns, this means those two insns
> > can be combined, as far as dataflow is concerned.  There never should
> > be a LOG_LINK between two unrelated insns.  If there is one, combine
> > will try to combine the insns without doing all the needed checks if
> > the earlier destination is used before the later insn, etc.
> > 
> > Unfortunately we do not update the LOG_LINKs correctly in some cases.
> > This patch fixes at least some of those cases.
> > 
> > This fixes the PR's testcase on aarch64.  Also tested on 30+ cross
> > compiler, and on powerpc64-linux {-m32,-m64}.  Will test on x86_64
> > as well before committing.
> 
> Will you check in the testcase too?

Yes, but it is dg-do run and I don't have a native aarch compiler built
for it yet, so that will be a later patch.

> My preference would be something like following, so that it can
> be torture-tested on all targets.

Thanks!


Segher
Segher Boessenkool Feb. 13, 2018, 10:15 p.m. | #3
On Mon, Feb 12, 2018 at 05:12:20PM +0100, Jakub Jelinek wrote:
> On Mon, Feb 12, 2018 at 03:59:05PM +0000, Segher Boessenkool wrote:
> > If there is a LOG_LINK between two insns, this means those two insns
> > can be combined, as far as dataflow is concerned.  There never should
> > be a LOG_LINK between two unrelated insns.  If there is one, combine
> > will try to combine the insns without doing all the needed checks if
> > the earlier destination is used before the later insn, etc.
> > 
> > Unfortunately we do not update the LOG_LINKs correctly in some cases.
> > This patch fixes at least some of those cases.
> > 
> > This fixes the PR's testcase on aarch64.  Also tested on 30+ cross
> > compiler, and on powerpc64-linux {-m32,-m64}.  Will test on x86_64
> > as well before committing.
> 
> Will you check in the testcase too?
> 
> My preference would be something like following, so that it can
> be torture-tested on all targets.

Works flawlessly everywhere.  I've committed this now, thanks again!


Segher
Jakub Jelinek Feb. 14, 2018, 8:37 p.m. | #4
On Mon, Feb 12, 2018 at 03:59:05PM +0000, Segher Boessenkool wrote:
> 2018-02-12  Segher Boessenkool  <segher@kernel.crashing.org>
> 
> 	PR rtl-optimization/84169
> 	* combine.c (try_combine): New variable split_i2i3.  Set it to true if
> 	we generated a parallel as new i3 and we split that to new i2 and i3
> 	instructions.  Handle split_i2i3 similar to swap_i2i3: scan the
> 	LOG_LINKs of i3 to see which of those need to link to i2 now.  Link
> 	those to i2, not i1.  Partially rewrite this scan code.

> +      unsigned int regno = REGNO (SET_DEST (x));

This line ICEs with rtl checking on both x86_64-linux and i686-linux
on gcc.c-torture/compile/pr66168.c:
+FAIL: gcc.c-torture/compile/pr66168.c   -O2  (internal compiler error)
+FAIL: gcc.c-torture/compile/pr66168.c   -O2  (test for excess errors)
+FAIL: gcc.c-torture/compile/pr66168.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  (internal compiler error)
+FAIL: gcc.c-torture/compile/pr66168.c   -O2 -flto -fno-use-linker-plugin -flto-partition=none  (test for excess errors)
+FAIL: gcc.c-torture/compile/pr66168.c   -O3 -g  (internal compiler error)
+FAIL: gcc.c-torture/compile/pr66168.c   -O3 -g  (test for excess errors)
+FAIL: gcc.c-torture/compile/pr66168.c   -Os  (internal compiler error)
+FAIL: gcc.c-torture/compile/pr66168.c   -Os  (test for excess errors)

/home/jakub/src/gcc/gcc/testsuite/gcc.c-torture/compile/pr66168.c:15:1: internal compiler error: RTL check: expected code 'reg', have 'subreg' in rhs_regno, at rtl.h:1896
0x6755ff rtl_check_failed_code1(rtx_def const*, rtx_code, char const*, int, char const*)
        ../../gcc/rtl.c:844
0x7fcdd2 rhs_regno
        ../../gcc/rtl.h:1896
0x807e75 rhs_regno
        ../../gcc/rtl.h:1447
0x807e75 try_combine
        ../../gcc/combine.c:4286
0x1680cc1 combine_instructions
        ../../gcc/combine.c:1320
0x1680cc1 rest_of_handle_combine
        ../../gcc/combine.c:14881
0x1680cc1 execute
        ../../gcc/combine.c:14926

	Jakub

Patch

diff --git a/gcc/combine.c b/gcc/combine.c
index 870bc77..204368e 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -2737,6 +2737,7 @@  try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
   /* Notes that I1, I2 or I3 is a MULT operation.  */
   int have_mult = 0;
   int swap_i2i3 = 0;
+  int split_i2i3 = 0;
   int changed_i3_dest = 0;
 
   int maxreg;
@@ -4167,6 +4168,9 @@  try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
 	    }
 
 	  insn_code_number = recog_for_combine (&newpat, i3, &new_i3_notes);
+
+	  if (insn_code_number >= 0)
+	    split_i2i3 = 1;
 	}
     }
 
@@ -4334,44 +4338,45 @@  try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
 
   if (swap_i2i3)
     {
-      rtx_insn *insn;
-      struct insn_link *link;
-      rtx ni2dest;
-
       /* I3 now uses what used to be its destination and which is now
 	 I2's destination.  This requires us to do a few adjustments.  */
       PATTERN (i3) = newpat;
       adjust_for_new_dest (i3);
+    }
 
-      /* We need a LOG_LINK from I3 to I2.  But we used to have one,
-	 so we still will.
+  if (swap_i2i3 || split_i2i3)
+    {
+      /* We might need a LOG_LINK from I3 to I2.  But then we used to
+	 have one, so we still will.
 
 	 However, some later insn might be using I2's dest and have
-	 a LOG_LINK pointing at I3.  We must remove this link.
-	 The simplest way to remove the link is to point it at I1,
-	 which we know will be a NOTE.  */
+	 a LOG_LINK pointing at I3.  We should change it to point at
+	 I2 instead.  */
 
       /* newi2pat is usually a SET here; however, recog_for_combine might
 	 have added some clobbers.  */
-      if (GET_CODE (newi2pat) == PARALLEL)
-	ni2dest = SET_DEST (XVECEXP (newi2pat, 0, 0));
-      else
-	ni2dest = SET_DEST (newi2pat);
+      rtx x = newi2pat;
+      if (GET_CODE (x) == PARALLEL)
+	x = XVECEXP (newi2pat, 0, 0);
 
-      for (insn = NEXT_INSN (i3);
-	   insn && (this_basic_block->next_bb == EXIT_BLOCK_PTR_FOR_FN (cfun)
-		    || insn != BB_HEAD (this_basic_block->next_bb));
+      unsigned int regno = REGNO (SET_DEST (x));
+
+      bool done = false;
+      for (rtx_insn *insn = NEXT_INSN (i3);
+	   !done
+	   && insn
+	   && NONDEBUG_INSN_P (insn)
+	   && BLOCK_FOR_INSN (insn) == this_basic_block;
 	   insn = NEXT_INSN (insn))
 	{
-	  if (NONDEBUG_INSN_P (insn)
-	      && reg_referenced_p (ni2dest, PATTERN (insn)))
-	    {
-	      FOR_EACH_LOG_LINK (link, insn)
-		if (link->insn == i3)
-		  link->insn = i1;
-
-	      break;
-	    }
+	  struct insn_link *link;
+	  FOR_EACH_LOG_LINK (link, insn)
+	    if (link->insn == i3 && link->regno == regno)
+	      {
+		link->insn = i2;
+		done = true;
+		break;
+	      }
 	}
     }