diff mbox series

Fix another reg-stack recovery bug (PR target/84828)

Message ID 20180312211043.GU8577@tucnak
State New
Headers show
Series Fix another reg-stack recovery bug (PR target/84828) | expand

Commit Message

Jakub Jelinek March 12, 2018, 9:10 p.m. UTC 
Hi!

As Martin reported, the same testcase added recently ICEs differently
with different options on x86_64-linux, the problem is that we
sometimes emit insns before a CODE_LABEL of the next bb rather than after
the last insn in the previous bb, and expect we can just fix up BB_END
of the previous bb; fortunately that doesn't change BB_HEAD of the next
bb, but still we need to at least update INSN_BLOCK and make sure df knows
that too.

Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

The testcase still ICEs on i686-linux (preexisting bug, ICEs the same
without this patch or even before the previous patch), will handle that
tomorrow.

2018-03-12  Jakub Jelinek  <jakub@redhat.com>

	PR target/84828
	* reg-stack.c (change_stack): Change update_end var from int to
	rtx_insn *, if non-NULL don't update just BB_END (current_block), but
	also call set_block_for_insn on the newly added insns and rescan.

	* g++.dg/ext/pr84828.C: New test.


	Jakub

Comments

Uros Bizjak March 13, 2018, 7:55 a.m. UTC | #1 
On Mon, Mar 12, 2018 at 10:10 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> As Martin reported, the same testcase added recently ICEs differently
> with different options on x86_64-linux, the problem is that we
> sometimes emit insns before a CODE_LABEL of the next bb rather than after
> the last insn in the previous bb, and expect we can just fix up BB_END
> of the previous bb; fortunately that doesn't change BB_HEAD of the next
> bb, but still we need to at least update INSN_BLOCK and make sure df knows
> that too.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?
>
> The testcase still ICEs on i686-linux (preexisting bug, ICEs the same
> without this patch or even before the previous patch), will handle that
> tomorrow.
>
> 2018-03-12  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/84828
>         * reg-stack.c (change_stack): Change update_end var from int to
>         rtx_insn *, if non-NULL don't update just BB_END (current_block), but
>         also call set_block_for_insn on the newly added insns and rescan.
>
>         * g++.dg/ext/pr84828.C: New test.

LGTM.

Uros.

> --- gcc/reg-stack.c.jj  2018-03-12 15:06:21.565800672 +0100
> +++ gcc/reg-stack.c     2018-03-12 15:06:31.167802560 +0100
> @@ -2489,7 +2489,7 @@ change_stack (rtx_insn *insn, stack_ptr
>               enum emit_where where)
>  {
>    int reg;
> -  int update_end = 0;
> +  rtx_insn *update_end = NULL;
>    int i;
>
>    /* Stack adjustments for the first insn in a block update the
> @@ -2511,7 +2511,7 @@ change_stack (rtx_insn *insn, stack_ptr
>    if (where == EMIT_AFTER)
>      {
>        if (current_block && BB_END (current_block) == insn)
> -       update_end = 1;
> +       update_end = insn;
>        insn = NEXT_INSN (insn);
>      }
>
> @@ -2686,7 +2686,16 @@ change_stack (rtx_insn *insn, stack_ptr
>      }
>
>    if (update_end)
> -    BB_END (current_block) = PREV_INSN (insn);
> +    {
> +      for (update_end = NEXT_INSN (update_end); update_end != insn;
> +          update_end = NEXT_INSN (update_end))
> +       {
> +         set_block_for_insn (update_end, current_block);
> +         if (INSN_P (update_end))
> +           df_insn_rescan (update_end);
> +       }
> +      BB_END (current_block) = PREV_INSN (insn);
> +    }
>  }
>
>  /* Print stack configuration.  */
> --- gcc/testsuite/g++.dg/ext/pr84828.C.jj       2018-03-12 15:13:50.589882983 +0100
> +++ gcc/testsuite/g++.dg/ext/pr84828.C  2018-03-12 15:14:06.670885916 +0100
> @@ -0,0 +1,13 @@
> +// PR target/84828
> +// { dg-do compile { target i?86-*-* x86_64-*-* } }
> +// { dg-options "-Og -mno-sse2" }
> +
> +void
> +foo (float b, double c)
> +{
> +  for (int e = 0; e < 2; e++)
> +    {
> +      asm volatile ("" : "+f" (c));    // { dg-error "must specify a single register" }
> +      asm ("" : "+rm" (c = b));
> +    }
> +}
>
>         Jakub
H.J. Lu March 13, 2018, 11:59 a.m. UTC | #2 
On Tue, Mar 13, 2018 at 12:55 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Mon, Mar 12, 2018 at 10:10 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> Hi!
>>
>> As Martin reported, the same testcase added recently ICEs differently
>> with different options on x86_64-linux, the problem is that we
>> sometimes emit insns before a CODE_LABEL of the next bb rather than after
>> the last insn in the previous bb, and expect we can just fix up BB_END
>> of the previous bb; fortunately that doesn't change BB_HEAD of the next
>> bb, but still we need to at least update INSN_BLOCK and make sure df knows
>> that too.
>>
>> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
>> trunk?
>>
>> The testcase still ICEs on i686-linux (preexisting bug, ICEs the same
>> without this patch or even before the previous patch), will handle that
>> tomorrow.
>>
>> 2018-03-12  Jakub Jelinek  <jakub@redhat.com>
>>
>>         PR target/84828
>>         * reg-stack.c (change_stack): Change update_end var from int to
>>         rtx_insn *, if non-NULL don't update just BB_END (current_block), but
>>         also call set_block_for_insn on the newly added insns and rescan.
>>
>>         * g++.dg/ext/pr84828.C: New test.
>
> LGTM.
>
> Uros.
>

I got:

spawn -ignore SIGHUP
/export/gnu/import/git/gcc-test/bld/gcc/testsuite/g++/../../xg++
-B/export/gnu/import/git/gcc-test/bld/gcc/testsuite/g++/../../
/export/gnu/import/git/gcc-test/src-trunk/gcc/testsuite/g++.dg/ext/pr84828.C
-fno-diagnostics-show-caret -fdiagnostics-color=never -nostdinc++
-I/export/gnu/import/git/gcc-test/bld/x86_64-pc-linux-gnu/libstdc++-v3/include/x86_64-pc-linux-gnu
-I/export/gnu/import/git/gcc-test/bld/x86_64-pc-linux-gnu/libstdc++-v3/include
-I/export/gnu/import/git/gcc-test/src-trunk/libstdc++-v3/libsupc++
-I/export/gnu/import/git/gcc-test/src-trunk/libstdc++-v3/include/backward
-I/export/gnu/import/git/gcc-test/src-trunk/libstdc++-v3/testsuite/util
-fmessage-length=0 -std=gnu++98 -Og -mno-sse2 -S -o pr84828.s^M
/export/gnu/import/git/gcc-test/src-trunk/gcc/testsuite/g++.dg/ext/pr84828.C:
In function 'void foo(float, double)':^M
/export/gnu/import/git/gcc-test/src-trunk/gcc/testsuite/g++.dg/ext/pr84828.C:10:35:
error: output constraint 0 must specify a single register^M
during RTL pass: stack^M
/export/gnu/import/git/gcc-test/src-trunk/gcc/testsuite/g++.dg/ext/pr84828.C:13:1:
internal compiler error: in move_for_stack_reg, at reg-stack.c:1108^M
0xeaf075 move_for_stack_reg^M
        ../../src-trunk/gcc/reg-stack.c:1108^M
0xeb26ea subst_stack_regs^M
        ../../src-trunk/gcc/reg-stack.c:2438^M
0xeb2950 convert_regs_1^M
        ../../src-trunk/gcc/reg-stack.c:3072^M
0xeb2950 convert_regs_2^M
        ../../src-trunk/gcc/reg-stack.c:3207^M
0xeb4798 convert_regs^M
        ../../src-trunk/gcc/reg-stack.c:3242^M
0xeb4798 reg_to_stack^M
        ../../src-trunk/gcc/reg-stack.c:3367^M
0xeb4798 rest_of_handle_stack_regs^M
        ../../src-trunk/gcc/reg-stack.c:3422^M
0xeb4798 execute^M
        ../../src-trunk/gcc/reg-stack.c:3453^M
Please submit a full bug report,^M
with preprocessed source if appropriate.^M
Please include the complete backtrace with any bug report.^M
See <https://gcc.gnu.org/bugs/> for instructions.^M
compiler exited with status 1
FAIL: g++.dg/ext/pr84828.C  -std=gnu++98 (internal compiler error)
PASS: g++.dg/ext/pr84828.C  -std=gnu++98  (test for errors, line 10)
FAIL: g++.dg/ext/pr84828.C  -std=gnu++98 (test for excess errors)
Jakub Jelinek March 13, 2018, 12:02 p.m. UTC | #3 
On Tue, Mar 13, 2018 at 04:59:58AM -0700, H.J. Lu wrote:
> >> The testcase still ICEs on i686-linux (preexisting bug, ICEs the same
> >> without this patch or even before the previous patch), will handle that
> >> tomorrow.

> I got:

See the above?
Or do you get this with -m64 too, rather than -m32 only?

	Jakub
H.J. Lu March 13, 2018, 12:13 p.m. UTC | #4 
On Tue, Mar 13, 2018 at 5:02 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Mar 13, 2018 at 04:59:58AM -0700, H.J. Lu wrote:
>> >> The testcase still ICEs on i686-linux (preexisting bug, ICEs the same
>> >> without this patch or even before the previous patch), will handle that
>> >> tomorrow.
>
>> I got:
>
> See the above?
> Or do you get this with -m64 too, rather than -m32 only?
>

I got this for -m32, -m64 and -mx32.
diff mbox series

Patch

--- gcc/reg-stack.c.jj	2018-03-12 15:06:21.565800672 +0100
+++ gcc/reg-stack.c	2018-03-12 15:06:31.167802560 +0100
@@ -2489,7 +2489,7 @@  change_stack (rtx_insn *insn, stack_ptr
 	      enum emit_where where)
 {
   int reg;
-  int update_end = 0;
+  rtx_insn *update_end = NULL;
   int i;
 
   /* Stack adjustments for the first insn in a block update the
@@ -2511,7 +2511,7 @@  change_stack (rtx_insn *insn, stack_ptr
   if (where == EMIT_AFTER)
     {
       if (current_block && BB_END (current_block) == insn)
-	update_end = 1;
+	update_end = insn;
       insn = NEXT_INSN (insn);
     }
 
@@ -2686,7 +2686,16 @@  change_stack (rtx_insn *insn, stack_ptr
     }
 
   if (update_end)
-    BB_END (current_block) = PREV_INSN (insn);
+    {
+      for (update_end = NEXT_INSN (update_end); update_end != insn;
+	   update_end = NEXT_INSN (update_end))
+	{
+	  set_block_for_insn (update_end, current_block);
+	  if (INSN_P (update_end))
+	    df_insn_rescan (update_end);
+	}
+      BB_END (current_block) = PREV_INSN (insn);
+    }
 }
 
 /* Print stack configuration.  */
--- gcc/testsuite/g++.dg/ext/pr84828.C.jj	2018-03-12 15:13:50.589882983 +0100
+++ gcc/testsuite/g++.dg/ext/pr84828.C	2018-03-12 15:14:06.670885916 +0100
@@ -0,0 +1,13 @@ 
+// PR target/84828
+// { dg-do compile { target i?86-*-* x86_64-*-* } }
+// { dg-options "-Og -mno-sse2" }
+
+void
+foo (float b, double c)
+{
+  for (int e = 0; e < 2; e++)
+    {
+      asm volatile ("" : "+f" (c));	// { dg-error "must specify a single register" }
+      asm ("" : "+rm" (c = b));
+    }
+}