[PATCH] Fix PR 51505

[PATCH] Fix PR 51505

[Andrey Belevantsev]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

Hello,

As discussed in Bugzilla, this is the patch implementing Paolo's suggestion 
of killing REG_EQUAL/REG_EQUIV notes from df_kill_notes.  The code assumes 
there is at most one such note per insn.  Bootstrapped and tested on 
x86-64, ok for trunk?

Andrey

gcc:
2012-01-18  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/51505
	* df-problems.c (df_kill_notes): New parameter live.  Update comment.
	Remove REG_EQUAL/REG_EQUIV notes referring to dead registers.
	(df_note_bb_compute): Update the call to df_kill_notes.

testsuite:
2012-01-18  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/51505
	* gcc.dg/pr51505.c: New test.

[-- Attachment #2: pr51505.diff --]
[-- Type: text/x-patch, Size: 2559 bytes --]

diff --git a/gcc/df-problems.c b/gcc/df-problems.c
index 8928454..f9b0bc1 100644
--- a/gcc/df-problems.c
+++ b/gcc/df-problems.c
@@ -2748,10 +2748,12 @@ df_ignore_stack_reg (int regno ATTRIBUTE_UNUSED)
 
 
 /* Remove all of the REG_DEAD or REG_UNUSED notes from INSN and add
-   them to OLD_DEAD_NOTES and OLD_UNUSED_NOTES.  */
+   them to OLD_DEAD_NOTES and OLD_UNUSED_NOTES.  Remove also
+   REG_EQUAL/REG_EQUIV notes referring to dead pseudos using LIVE
+   as the bitmap of currently live registers.  */
 
 static void
-df_kill_notes (rtx insn)
+df_kill_notes (rtx insn, bitmap live)
 {
   rtx *pprev = &REG_NOTES (insn);
   rtx link = *pprev;
@@ -2798,6 +2800,45 @@ df_kill_notes (rtx insn)
 	    }
 	  break;
 
+	case REG_EQUAL:
+	case REG_EQUIV:
+	  {
+	    /* Remove the notes that refer to dead registers.  As we have at most
+	       one REG_EQUAL/EQUIV note, all of EQ_USES will refer to this note
+	       so we need to purge the complete EQ_USES vector when removing
+	       the note using df_notes_rescan.  */
+	    df_ref *use_rec;
+	    bool deleted = false;
+
+	    for (use_rec = DF_INSN_EQ_USES (insn); *use_rec; use_rec++)
+	      {
+		df_ref use = *use_rec;
+		if (DF_REF_REGNO (use) > FIRST_PSEUDO_REGISTER
+		    && (DF_REF_FLAGS (use) & DF_REF_IN_NOTE)
+		    && ! bitmap_bit_p (live, DF_REF_REGNO (use)))
+		  {
+		    deleted = true;
+		    break;
+		  }
+	      }
+	    if (deleted)
+	      {
+		rtx next;
+#ifdef REG_DEAD_DEBUGGING
+		df_print_note ("deleting: ", insn, link);
+#endif
+		next = XEXP (link, 1);
+		free_EXPR_LIST_node (link);
+		*pprev = link = next;
+		df_notes_rescan (insn);
+	      }
+	    else
+	      {
+		pprev = &XEXP (link, 1);
+		link = *pprev;
+	      }
+	    break;
+	  }
 	default:
 	  pprev = &XEXP (link, 1);
 	  link = *pprev;
@@ -3299,7 +3340,7 @@ df_note_bb_compute (unsigned int bb_index,
       debug_insn = DEBUG_INSN_P (insn);
 
       bitmap_clear (do_not_gen);
-      df_kill_notes (insn);
+      df_kill_notes (insn, live);
 
       /* Process the defs.  */
       if (CALL_P (insn))
diff --git a/gcc/testsuite/gcc.dg/pr51505.c b/gcc/testsuite/gcc.dg/pr51505.c
new file mode 100644
index 0000000..dbcd322
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr51505.c
@@ -0,0 +1,19 @@
+/* PR rtl-optimization/51505 */
+/* { dg-do compile } */
+/* { dg-options "-O --param max-cse-insns=1" } */
+struct S
+{
+char a[256];
+};
+
+int bar(struct S, char[16]);
+
+void foo ()
+{
+  struct S u, s1, s2;
+  char e[256];
+  char i;
+  e[i] = ~s1.a[i] & s2.a[i];
+  if (bar(u, e))
+    __builtin_abort ();
+}

Re: [PATCH] Fix PR 51505

[Paolo Bonzini]

On 01/18/2012 05:41 PM, Andrey Belevantsev wrote:
> Hello,
>
> As discussed in Bugzilla, this is the patch implementing Paolo's
> suggestion of killing REG_EQUAL/REG_EQUIV notes from df_kill_notes. The
> code assumes there is at most one such note per insn. Bootstrapped and
> tested on x86-64, ok for trunk?
>
> Andrey
>
> gcc:
> 2012-01-18 Andrey Belevantsev <abel@ispras.ru>
>
> PR rtl-optimization/51505
> * df-problems.c (df_kill_notes): New parameter live. Update comment.
> Remove REG_EQUAL/REG_EQUIV notes referring to dead registers.
> (df_note_bb_compute): Update the call to df_kill_notes.
>
> testsuite:
> 2012-01-18 Andrey Belevantsev <abel@ispras.ru>
>
> PR rtl-optimization/51505
> * gcc.dg/pr51505.c: New test.

Ok, thanks for working on this.

Paolo

Re: [PATCH] Fix PR 51505

[Andrey Belevantsev]

On 18.01.2012 21:28, Paolo Bonzini wrote:
> On 01/18/2012 05:41 PM, Andrey Belevantsev wrote:
> Ok, thanks for working on this.
Installed, do you want this for 4.6/4.5?

Andrey

Re: [PATCH] Fix PR 51505

[Jakub Jelinek]

On Thu, Jan 19, 2012 at 11:32:41AM +0400, Andrey Belevantsev wrote:
> On 18.01.2012 21:28, Paolo Bonzini wrote:
> >On 01/18/2012 05:41 PM, Andrey Belevantsev wrote:
> >Ok, thanks for working on this.
> Installed, do you want this for 4.6/4.5?

If yes, please give it at least a couple of weeks on the trunk.

	Jakub

Re: [PATCH] Fix PR 51505

[Paolo Bonzini]

On 01/19/2012 08:34 AM, Jakub Jelinek wrote:
>>> >  >Ok, thanks for working on this.
>> >  Installed, do you want this for 4.6/4.5?
> If yes, please give it at least a couple of weeks on the trunk.

It's fine by me but yes, let's give it time to bake.

Paolo

Re: [PATCH] Fix PR 51505

[Eric Botcazou]

> As discussed in Bugzilla, this is the patch implementing Paolo's suggestion
> of killing REG_EQUAL/REG_EQUIV notes from df_kill_notes.  The code assumes
> there is at most one such note per insn.

That's wrong though and wreaks havoc during reload, e.g.:

(insn 169 60 62 4 (set (reg:TF 158)
        (mem/c:TF (plus:SI (reg/f:SI 101 %sfp)
                (const_int -16 [0xfffffffffffffff0])) [3 S16 A64])) 
960513-1.c:13 97 {*movtf_insn_sp32}
     (expr_list:REG_EQUIV (mem/c:TF (plus:SI (reg/f:SI 101 %sfp)
                (const_int -16 [0xfffffffffffffff0])) [3 S16 A64])
        (expr_list:REG_EQUAL (mult:TF (reg/v:TF 110 [ d ])
                (reg:TF 154))
            (nil))))

because the REG_EQUIV note disappears behind reload's back and it isn't 
prepared for that.  This is the cause of the following regression on SPARC:

FAIL: gcc.c-torture/execute/960513-1.c execution,  -Os

-- 
Eric Botcazou

Re: [PATCH] Fix PR 51505

[Eric Botcazou]

> > As discussed in Bugzilla, this is the patch implementing Paolo's
> > suggestion of killing REG_EQUAL/REG_EQUIV notes from df_kill_notes.  The
> > code assumes there is at most one such note per insn.
>
> That's wrong though and wreaks havoc during reload, e.g.:
>
> (insn 169 60 62 4 (set (reg:TF 158)
>         (mem/c:TF (plus:SI (reg/f:SI 101 %sfp)
>                 (const_int -16 [0xfffffffffffffff0])) [3 S16 A64]))
> 960513-1.c:13 97 {*movtf_insn_sp32}
>      (expr_list:REG_EQUIV (mem/c:TF (plus:SI (reg/f:SI 101 %sfp)
>                 (const_int -16 [0xfffffffffffffff0])) [3 S16 A64])
>         (expr_list:REG_EQUAL (mult:TF (reg/v:TF 110 [ d ])
>                 (reg:TF 154))
>             (nil))))
>
> because the REG_EQUIV note disappears behind reload's back and it isn't
> prepared for that.  This is the cause of the following regression on SPARC:
>
> FAIL: gcc.c-torture/execute/960513-1.c execution,  -Os

As well as:

FAIL: gcc.c-torture/execute/stdarg-2.c execution,  -O2 
FAIL: gcc.c-torture/execute/stdarg-2.c execution,  -O3 -fomit-frame-pointer 
FAIL: gcc.c-torture/execute/stdarg-2.c execution,  -O3 -g 
FAIL: gcc.c-torture/execute/stdarg-2.c execution,  -Os 
FAIL: gcc.c-torture/execute/stdarg-2.c 
execution,  -O2 -flto -flto-partition=none 
FAIL: gcc.c-torture/execute/stdarg-2.c execution,  -O2 -flto 

for the exact same reason.

-- 
Eric Botcazou

Re: [PATCH] Fix PR 51505

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

On 01/29/2012 04:09 PM, Eric Botcazou wrote:
>>> As discussed in Bugzilla, this is the patch implementing Paolo's
>>> suggestion of killing REG_EQUAL/REG_EQUIV notes from df_kill_notes.  The
>>> code assumes there is at most one such note per insn.
>>
>> That's wrong though and wreaks havoc during reload, e.g.:
>>
>> (insn 169 60 62 4 (set (reg:TF 158)
>>          (mem/c:TF (plus:SI (reg/f:SI 101 %sfp)
>>                  (const_int -16 [0xfffffffffffffff0])) [3 S16 A64]))
>> 960513-1.c:13 97 {*movtf_insn_sp32}
>>       (expr_list:REG_EQUIV (mem/c:TF (plus:SI (reg/f:SI 101 %sfp)
>>                  (const_int -16 [0xfffffffffffffff0])) [3 S16 A64])
>>          (expr_list:REG_EQUAL (mult:TF (reg/v:TF 110 [ d ])
>>                  (reg:TF 154))
>>              (nil))))
>>
>> because the REG_EQUIV note disappears behind reload's back and it isn't
>> prepared for that.  This is the cause of the following regression on SPARC:
>>
>> FAIL: gcc.c-torture/execute/960513-1.c execution,  -Os
>
> As well as:
>
> FAIL: gcc.c-torture/execute/stdarg-2.c execution,  -O2
> FAIL: gcc.c-torture/execute/stdarg-2.c execution,  -O3 -fomit-frame-pointer
> FAIL: gcc.c-torture/execute/stdarg-2.c execution,  -O3 -g
> FAIL: gcc.c-torture/execute/stdarg-2.c execution,  -Os
> FAIL: gcc.c-torture/execute/stdarg-2.c
> execution,  -O2 -flto -flto-partition=none
> FAIL: gcc.c-torture/execute/stdarg-2.c execution,  -O2 -flto
>
> for the exact same reason.

Does this help?

Paolo


[-- Attachment #2: df-problems.patch --]
[-- Type: text/x-patch, Size: 733 bytes --]

2012-01-30  Paolo Bonzini  <bonzini@gnu.org>

	* df-problems.c (df_kill_notes): Check that the use refers
	to the note under examination.

Index: df-problems.c
===================================================================
--- df-problems.c	(revision 183693)
+++ df-problems.c	(working copy)
@@ -2814,8 +2814,10 @@ df_kill_notes (rtx insn, bitmap live)
 	      {
 		df_ref use = *use_rec;
 		if (DF_REF_REGNO (use) > FIRST_PSEUDO_REGISTER
+		    && DF_REF_LOC (use)
 		    && (DF_REF_FLAGS (use) & DF_REF_IN_NOTE)
-		    && ! bitmap_bit_p (live, DF_REF_REGNO (use)))
+		    && ! bitmap_bit_p (live, DF_REF_REGNO (use))
+		    && loc_mentioned_in_p (DF_REF_LOC (use), XEXP (link, 0)))
 		  {
 		    deleted = true;
 		    break;

Re: [PATCH] Fix PR 51505

[Andrey Belevantsev]

On 30.01.2012 11:38, Paolo Bonzini wrote:
> On 01/29/2012 04:09 PM, Eric Botcazou wrote:
>>>> As discussed in Bugzilla, this is the patch implementing Paolo's
>>>> suggestion of killing REG_EQUAL/REG_EQUIV notes from df_kill_notes. The
>>>> code assumes there is at most one such note per insn.
>>>
>>> That's wrong though and wreaks havoc during reload, e.g.:
>>>
>>> (insn 169 60 62 4 (set (reg:TF 158)
>>> (mem/c:TF (plus:SI (reg/f:SI 101 %sfp)
>>> (const_int -16 [0xfffffffffffffff0])) [3 S16 A64]))
>>> 960513-1.c:13 97 {*movtf_insn_sp32}
>>> (expr_list:REG_EQUIV (mem/c:TF (plus:SI (reg/f:SI 101 %sfp)
>>> (const_int -16 [0xfffffffffffffff0])) [3 S16 A64])
>>> (expr_list:REG_EQUAL (mult:TF (reg/v:TF 110 [ d ])
>>> (reg:TF 154))
>>> (nil))))
>>>
>>> because the REG_EQUIV note disappears behind reload's back and it isn't
>>> prepared for that. This is the cause of the following regression on SPARC:
>>>
>>> FAIL: gcc.c-torture/execute/960513-1.c execution, -Os
>>
>> As well as:
>>
>> FAIL: gcc.c-torture/execute/stdarg-2.c execution, -O2
>> FAIL: gcc.c-torture/execute/stdarg-2.c execution, -O3 -fomit-frame-pointer
>> FAIL: gcc.c-torture/execute/stdarg-2.c execution, -O3 -g
>> FAIL: gcc.c-torture/execute/stdarg-2.c execution, -Os
>> FAIL: gcc.c-torture/execute/stdarg-2.c
>> execution, -O2 -flto -flto-partition=none
>> FAIL: gcc.c-torture/execute/stdarg-2.c execution, -O2 -flto
>>
>> for the exact same reason.
>
> Does this help?

That would fix the problem of multiple notes per insn (as we wanted to do 
initially), but I didn't understand whether this is the real problem or the 
problem is the reload not happy with disappearing notes.  Also I can't 
reproduce it with a cross to sparc64-linux -- when I put a breakpoint on 
the code removing the notes, I find only similarly looking insn 148 which 
gets removed from the df_analyze call at the start of IRA.  Though I see 
the fail from SPARC test results on the ML, so I guess I'm missing something...

Andrey

>
> Paolo
>

Re: [PATCH] Fix PR 51505

[Eric Botcazou]

> That would fix the problem of multiple notes per insn (as we wanted to do
> initially), but I didn't understand whether this is the real problem or the
> problem is the reload not happy with disappearing notes.

reload maintains a mapping between its internal structures and the RTL stream, 
here between reg_equiv_* fields and REG_EQUIV notes.  It simply isn't prepared 
to deal with the case where this mapping is broken behind its back.

-- 
Eric Botcazou

Re: [PATCH] Fix PR 51505

[Eric Botcazou]

> Does this help?

Yep, this eliminates all the regressions, thanks!

-- 
Eric Botcazou

Re: [PATCH] Fix PR 51505

[Paolo Bonzini]

On 01/30/2012 01:22 PM, Eric Botcazou wrote:
>> Does this help?
> Yep, this eliminates all the regressions, thanks!

Committed as r183719.  Thanks for testing.

Paolo

Re: [PATCH] Fix PR 51505

[Paolo Bonzini]

On 01/30/2012 09:44 AM, Andrey Belevantsev wrote:
>>
>> Does this help?
>
> That would fix the problem of multiple notes per insn (as we wanted to
> do initially), but I didn't understand whether this is the real problem
> or the problem is the reload not happy with disappearing notes.  Also I
> can't reproduce it with a cross to sparc64-linux -- when I put a
> breakpoint on the code removing the notes, I find only similarly looking
> insn 148 which gets removed from the df_analyze call at the start of
> IRA.  Though I see the fail from SPARC test results on the ML, so I
> guess I'm missing something...

The REG_EQUAL note can go, but the REG_EQUIV note should not (by 
definition: they are valid throughout the entire function).  In fact, we 
could just as well apply the loop to REG_EQUAL notes only but that would 
have been a bit too clever and more risky.

Paolo

Re: [PATCH] Fix PR 51505

[Andrey Belevantsev]

On 30.01.2012 17:47, Paolo Bonzini wrote:
> On 01/30/2012 09:44 AM, Andrey Belevantsev wrote:
>>>
>>> Does this help?
>>
>> That would fix the problem of multiple notes per insn (as we wanted to
>> do initially), but I didn't understand whether this is the real problem
>> or the problem is the reload not happy with disappearing notes. Also I
>> can't reproduce it with a cross to sparc64-linux -- when I put a
>> breakpoint on the code removing the notes, I find only similarly looking
>> insn 148 which gets removed from the df_analyze call at the start of
>> IRA. Though I see the fail from SPARC test results on the ML, so I
>> guess I'm missing something...
>
> The REG_EQUAL note can go, but the REG_EQUIV note should not (by
> definition: they are valid throughout the entire function). In fact, we
> could just as well apply the loop to REG_EQUAL notes only but that would
> have been a bit too clever and more risky.

Eric, Paolo, thanks for the explanations!

Andrey