diff mbox

Fix PR48389: ICE in make_edges

Message ID Pine.LNX.4.64.1104081524260.1989@wotan.suse.de
State New
Headers show

Commit Message

Michael Matz April 8, 2011, 1:33 p.m. UTC 
Hi,

the problem is that some initializers expand to loops.  If such 
initializers are inserted on edges we have looping control flow on them.  
Even if we have no loops, but normal control flow on them (which can 
happen in other situations) we still forget to initialize JUMP_LABEL on 
them leading to potential problems downstream (no idea why we never hit 
any real problems until we had loops on edges, this problem is latent 
since expand from SSA form).

So, we have to initialize the jump labels somewhen.  The obvious idea to 
simply call rebuild_jump_labels after all insertions are done doesn't 
work, because the jump labels are already needed for splitting edges (for 
patching the jump insns), which insertion might cause.

So, instead than this I've added another interface to initialize jump 
labels for a non-main chain of instructions (in fact it's the same as with 
the main chain, merely not diddling with the forced_labels list).

Fixes the testcase, gcc dg.exp is clean, I'm currently regstrapping this 
on x86_64-linux.  Okay if that passes?


Ciao,
Michael.
	PR middle-end/48389
	* jump.c (rebuild_jump_labels_1, rebuild_jump_labels_chain): New
	functions.
	(rebuild_jump_labels): Call rebuild_jump_labels_1.
	* rtl.h (rebuild_jump_labels_chain): Declare.
	* cfgexpand.c (expand_gimple_basic_block): Use PAT_VAR_LOCATION_LOC,
	not INSN_VAR_LOCATION_LOC.
	(gimple_expand_cfg): Initialize JUMP_LABEL also on insn inserted
	on edges.

testsuite/
	* gcc.target/i386/pr48389.c: New test.

Comments

Jakub Jelinek April 8, 2011, 1:37 p.m. UTC | #1 
On Fri, Apr 08, 2011 at 03:33:49PM +0200, Michael Matz wrote:
> --- testsuite/gcc.target/i386/pr48389.c	(revision 0)
> +++ testsuite/gcc.target/i386/pr48389.c	(revision 0)
> @@ -0,0 +1,12 @@
> +/* PR middle-end/48389 */
> +/* { dg-do compile } */
> +/* { dg-options "-O -m32 -mtune=pentiumpro -Wno-abi" } */

-m32/-m64 should never go into dg-options.  Either do something like:
/* { dg-options "-O" } */
/* { dg-options "-O -mtune=pentiumpro -Wno-abi" { target ilp32 } } */
or remove -m32 from dg-options and conditionalize dg-do compile on ilp32
target.

	Jakub
Michael Matz April 8, 2011, 1:58 p.m. UTC | #2 
Hi,

On Fri, 8 Apr 2011, Jakub Jelinek wrote:

> On Fri, Apr 08, 2011 at 03:33:49PM +0200, Michael Matz wrote:
> > --- testsuite/gcc.target/i386/pr48389.c	(revision 0)
> > +++ testsuite/gcc.target/i386/pr48389.c	(revision 0)
> > @@ -0,0 +1,12 @@
> > +/* PR middle-end/48389 */
> > +/* { dg-do compile } */
> > +/* { dg-options "-O -m32 -mtune=pentiumpro -Wno-abi" } */
> 
> -m32/-m64 should never go into dg-options.  Either do something like:
> /* { dg-options "-O" } */
> /* { dg-options "-O -mtune=pentiumpro -Wno-abi" { target ilp32 } } */
> or remove -m32 from dg-options and conditionalize dg-do compile on ilp32
> target.

But then I'd have to use --target_board to hit the original problem.  
Can't I somehow make it so that independend of the target_board setting 
32bit code is generated (possibly only when the compiler supports it)?
If it's not possible consider the testcase to be adjusted like below.


Ciao,
Michael.
Jakub Jelinek April 8, 2011, 2:03 p.m. UTC | #3 
On Fri, Apr 08, 2011 at 03:58:57PM +0200, Michael Matz wrote:
> But then I'd have to use --target_board to hit the original problem.  

That isn't so a big deal, testing with
RUNTESTFLAGS='--target_board=unix\{-m32,-m64\}'
is what people do very often.  Or just do a 32-bit instead of 64-bit
bootstrap/regtest, which is easily possible even on x86_64-linux distro.

> Can't I somehow make it so that independend of the target_board setting 
> 32bit code is generated (possibly only when the compiler supports it)?

Not that I'm aware of.

	Jakub
H.J. Lu April 8, 2011, 2:55 p.m. UTC | #4 
On Fri, Apr 8, 2011 at 6:58 AM, Michael Matz <matz@suse.de> wrote:
> Hi,
>
> On Fri, 8 Apr 2011, Jakub Jelinek wrote:
>
>> On Fri, Apr 08, 2011 at 03:33:49PM +0200, Michael Matz wrote:
>> > --- testsuite/gcc.target/i386/pr48389.c     (revision 0)
>> > +++ testsuite/gcc.target/i386/pr48389.c     (revision 0)
>> > @@ -0,0 +1,12 @@
>> > +/* PR middle-end/48389 */
>> > +/* { dg-do compile } */
>> > +/* { dg-options "-O -m32 -mtune=pentiumpro -Wno-abi" } */
>>
>> -m32/-m64 should never go into dg-options.  Either do something like:
>> /* { dg-options "-O" } */
>> /* { dg-options "-O -mtune=pentiumpro -Wno-abi" { target ilp32 } } */
>> or remove -m32 from dg-options and conditionalize dg-do compile on ilp32
>> target.
>
> But then I'd have to use --target_board to hit the original problem.
> Can't I somehow make it so that independend of the target_board setting
> 32bit code is generated (possibly only when the compiler supports it)?
> If it's not possible consider the testcase to be adjusted like below.
>
>

You SHOULD use --target_board to test both 32bit and 64bit on
Linux/x86-64.
Jeff Law April 8, 2011, 3:04 p.m. UTC | #5 
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/08/11 07:33, Michael Matz wrote:
> Hi,
> 
> the problem is that some initializers expand to loops.  If such 
> initializers are inserted on edges we have looping control flow on them.  
> Even if we have no loops, but normal control flow on them (which can 
> happen in other situations) we still forget to initialize JUMP_LABEL on 
> them leading to potential problems downstream (no idea why we never hit 
> any real problems until we had loops on edges, this problem is latent 
> since expand from SSA form).
> 
> So, we have to initialize the jump labels somewhen.  The obvious idea to 
> simply call rebuild_jump_labels after all insertions are done doesn't 
> work, because the jump labels are already needed for splitting edges (for 
> patching the jump insns), which insertion might cause.
> 
> So, instead than this I've added another interface to initialize jump 
> labels for a non-main chain of instructions (in fact it's the same as with 
> the main chain, merely not diddling with the forced_labels list).
> 
> Fixes the testcase, gcc dg.exp is clean, I'm currently regstrapping this 
> on x86_64-linux.  Okay if that passes?
> 
> 
> Ciao,
> Michael.
> 	PR middle-end/48389
> 	* jump.c (rebuild_jump_labels_1, rebuild_jump_labels_chain): New
> 	functions.
> 	(rebuild_jump_labels): Call rebuild_jump_labels_1.
> 	* rtl.h (rebuild_jump_labels_chain): Declare.
> 	* cfgexpand.c (expand_gimple_basic_block): Use PAT_VAR_LOCATION_LOC,
> 	not INSN_VAR_LOCATION_LOC.
> 	(gimple_expand_cfg): Initialize JUMP_LABEL also on insn inserted
> 	on edges.
> 
> testsuite/
> 	* gcc.target/i386/pr48389.c: New test.
I'd pondered a similar approach, but hadn't tried an implementation.
Good to see it wasn't hard to make it work.  I like this better than
Steven's approach.  OK by me.

Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJNnyPuAAoJEBRtltQi2kC7us4H/02MnP3HKapVMu/DoXAwyxBJ
6lN8xSdFY7Z/DQbHnU9SiKvB8UlcPQs1HHN4MiFrNZ23n0Qhtd4Z4bphbnzb2cXs
gayymOheYGlEy7U/YHTUCjqOryMZbkwYybNbrFTlkl4d5Ymp2HEphZHn8G/8sRIr
9LY4o++M08zvZwX5WGd/NFJ9UosxCayhmfvA+raSwAuoz00EgD2Ns5KiqsLIhSey
A9RxKkV2Czo2piqdO2wlv0NtVzQMAyDsDU5pv+1zec6Vf1UuuZzLslPGVdVyNQqk
NCoz5gO8AyPLiMuNp57W0RtLtXHdwwGGVEv3y5AZ7T4bVvTVlAUuFhixeAc5tQ0=
=jS34
-----END PGP SIGNATURE-----
diff mbox

Patch

Index: jump.c
===================================================================
--- jump.c	(revision 171901)
+++ jump.c	(working copy)
@@ -72,12 +72,9 @@  static void redirect_exp_1 (rtx *, rtx,
 static int invert_exp_1 (rtx, rtx);
 static int returnjump_p_1 (rtx *, void *);
 
-/* This function rebuilds the JUMP_LABEL field and REG_LABEL_TARGET
-   notes in jumping insns and REG_LABEL_OPERAND notes in non-jumping
-   instructions and jumping insns that have labels as operands
-   (e.g. cbranchsi4).  */
-void
-rebuild_jump_labels (rtx f)
+/* Worker for rebuild_jump_labels and rebuild_jump_labels_chain.  */
+static void
+rebuild_jump_labels_1 (rtx f, bool count_forced)
 {
   rtx insn;
 
@@ -89,11 +86,31 @@  rebuild_jump_labels (rtx f)
      closely enough to delete them here, so make sure their reference
      count doesn't drop to zero.  */
 
-  for (insn = forced_labels; insn; insn = XEXP (insn, 1))
-    if (LABEL_P (XEXP (insn, 0)))
-      LABEL_NUSES (XEXP (insn, 0))++;
+  if (count_forced)
+    for (insn = forced_labels; insn; insn = XEXP (insn, 1))
+      if (LABEL_P (XEXP (insn, 0)))
+	LABEL_NUSES (XEXP (insn, 0))++;
   timevar_pop (TV_REBUILD_JUMP);
 }
+
+/* This function rebuilds the JUMP_LABEL field and REG_LABEL_TARGET
+   notes in jumping insns and REG_LABEL_OPERAND notes in non-jumping
+   instructions and jumping insns that have labels as operands
+   (e.g. cbranchsi4).  */
+void
+rebuild_jump_labels (rtx f)
+{
+  rebuild_jump_labels_1 (f, true);
+}
+
+/* This function is like rebuild_jump_labels, but doesn't run over
+   forced_labels.  It can be used on insn chains that aren't the 
+   main function chain.  */
+void
+rebuild_jump_labels_chain (rtx chain)
+{
+  rebuild_jump_labels_1 (chain, false);
+}
 
 /* Some old code expects exactly one BARRIER as the NEXT_INSN of a
    non-fallthru insn.  This is not generally true, as multiple barriers
Index: cfgexpand.c
===================================================================
--- cfgexpand.c	(revision 171901)
+++ cfgexpand.c	(working copy)
@@ -3582,9 +3582,9 @@  expand_gimple_basic_block (basic_block b
 		{
 		  /* We can't dump the insn with a TREE where an RTX
 		     is expected.  */
-		  INSN_VAR_LOCATION_LOC (val) = const0_rtx;
+		  PAT_VAR_LOCATION_LOC (val) = const0_rtx;
 		  maybe_dump_rtl_for_gimple_stmt (stmt, last);
-		  INSN_VAR_LOCATION_LOC (val) = (rtx)value;
+		  PAT_VAR_LOCATION_LOC (val) = (rtx)value;
 		}
 
 	      /* In order not to generate too many debug temporaries,
@@ -4143,6 +4143,8 @@  gimple_expand_cfg (void)
   /* Zap the tree EH table.  */
   set_eh_throw_stmt_table (cfun, NULL);
 
+  /* We need JUMP_LABEL be set in order to redirect jumps, and hence
+     split edges which edge insertions might do.  */
   rebuild_jump_labels (get_insns ());
 
   FOR_BB_BETWEEN (bb, ENTRY_BLOCK_PTR, EXIT_BLOCK_PTR, next_bb)
@@ -4153,6 +4155,7 @@  gimple_expand_cfg (void)
 	{
 	  if (e->insns.r)
 	    {
+	      rebuild_jump_labels_chain (e->insns.r);
 	      /* Avoid putting insns before parm_birth_insn.  */
 	      if (e->src == ENTRY_BLOCK_PTR
 		  && single_succ_p (ENTRY_BLOCK_PTR)
Index: rtl.h
===================================================================
--- rtl.h	(revision 171901)
+++ rtl.h	(working copy)
@@ -2315,6 +2315,7 @@  extern int redirect_jump_1 (rtx, rtx);
 extern void redirect_jump_2 (rtx, rtx, rtx, int, int);
 extern int redirect_jump (rtx, rtx, int);
 extern void rebuild_jump_labels (rtx);
+extern void rebuild_jump_labels_chain (rtx);
 extern rtx reversed_comparison (const_rtx, enum machine_mode);
 extern enum rtx_code reversed_comparison_code (const_rtx, const_rtx);
 extern enum rtx_code reversed_comparison_code_parts (enum rtx_code, const_rtx,
Index: testsuite/gcc.target/i386/pr48389.c
===================================================================
--- testsuite/gcc.target/i386/pr48389.c	(revision 0)
+++ testsuite/gcc.target/i386/pr48389.c	(revision 0)
@@ -0,0 +1,12 @@ 
+/* PR middle-end/48389 */
+/* { dg-do compile } */
+/* { dg-options "-O -m32 -mtune=pentiumpro -Wno-abi" } */
+typedef float V2SF __attribute__ ((vector_size (128)));
+V2SF foo (int x, V2SF a)
+{
+  V2SF b = {};
+  if (x & 42)
+    b = a;
+  a += b;
+  return a;
+}