Patchwork Fix PR 51505

login
register
mail settings
Submitter Paolo Bonzini
Date Jan. 30, 2012, 7:38 a.m.
Message ID <4F2648FA.8040702@gnu.org>
Download mbox | patch
Permalink /patch/138517/
State New
Headers show

Comments

Paolo Bonzini - Jan. 30, 2012, 7:38 a.m.
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
Andrey Belevantsev - Jan. 30, 2012, 8:44 a.m.
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
>
Eric Botcazou - Jan. 30, 2012, 12:22 p.m.
> Does this help?

Yep, this eliminates all the regressions, thanks!
Eric Botcazou - Jan. 30, 2012, 12:25 p.m.
> 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.
Paolo Bonzini - Jan. 30, 2012, 12:56 p.m.
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
Paolo Bonzini - Jan. 30, 2012, 1:47 p.m.
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
Andrey Belevantsev - Jan. 30, 2012, 2:18 p.m.
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

Patch

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;