Patchwork Fix for postreload

login
register
mail settings
Submitter Chung-Lin Tang
Date Sept. 13, 2010, 6:31 a.m.
Message ID <4C8DC542.30902@codesourcery.com>
Download mbox | patch
Permalink /patch/64574/
State New
Headers show

Comments

Chung-Lin Tang - Sept. 13, 2010, 6:31 a.m.
Hi,
this patch is a fix for the move2add transform in the postreload pass; 
reg_symbol_ref[] was not checked together for some reg_base_reg[] < 0 cases.

This was tested to fix a few gcc.dg/torture/fp-int-convert-* execution 
FAIL cases on trunk for ARM, with no other regressions.

Ok for trunk?

Thanks,
Chung-Lin

2010-09-13  Chung-Lin Tang  <cltang@codesourcery.com>

	* postreload.c (move2add_note_store): Add reg_symbol_ref[]
	checks to update conditions.
Eric Botcazou - Sept. 13, 2010, 3:06 p.m.
> this patch is a fix for the move2add transform in the postreload pass;
> reg_symbol_ref[] was not checked together for some reg_base_reg[] < 0
> cases.

It looks like there is another problem in the second check: shouldn't it be 
reg_mode[REGNO (base_reg)] instead of reg_mode[REGNO (XEXP (src, 1))]?  The 
check as currently written is redundant with the enclosing condition.

> 2010-09-13  Chung-Lin Tang  <cltang@codesourcery.com>
>
> 	* postreload.c (move2add_note_store): Add reg_symbol_ref[]
> 	checks to update conditions.

	* postreload.c (move2add_note_store) <PLUS>: ...
Chung-Lin Tang - Sept. 13, 2010, 4:37 p.m.
Eric Botcazou wrote:
>> this patch is a fix for the move2add transform in the postreload pass;
>> reg_symbol_ref[] was not checked together for some reg_base_reg[]<  0
>> cases.
>
> It looks like there is another problem in the second check: shouldn't it be
> reg_mode[REGNO (base_reg)] instead of reg_mode[REGNO (XEXP (src, 1))]?  The
> check as currently written is redundant with the enclosing condition.

I think it's because the second else-if clause exchanges base_reg (which 
is here known to be a constant) and XEXP(src,1), the latter becoming the 
new "base reg" below. So reg_mode[REGNO(XEXP(src,1))] is what matters here.
Chung-Lin Tang - Sept. 13, 2010, 4:49 p.m.
Chung-Lin Tang wrote:
> Eric Botcazou wrote:
>>> this patch is a fix for the move2add transform in the postreload pass;
>>> reg_symbol_ref[] was not checked together for some reg_base_reg[]<  0
>>> cases.
>>
>> It looks like there is another problem in the second check: shouldn't 
>> it be
>> reg_mode[REGNO (base_reg)] instead of reg_mode[REGNO (XEXP (src, 
>> 1))]?  The
>> check as currently written is redundant with the enclosing condition.
>
> I think it's because the second else-if clause exchanges base_reg 
> (which is here known to be a constant) and XEXP(src,1), the latter 
> becoming the new "base reg" below. So reg_mode[REGNO(XEXP(src,1))] is 
> what matters here.
>
>
Sorry, I think I missed your point about it being redundant; the outer 
reg_mode[REGNO(XEXP(src,1))] check does seem to be removable. I'll 
update and test this later.
Eric Botcazou - Sept. 13, 2010, 6:13 p.m.
> I think it's because the second else-if clause exchanges base_reg (which
> is here known to be a constant) and XEXP(src,1), the latter becoming the
> new "base reg" below. So reg_mode[REGNO(XEXP(src,1))] is what matters here.

I think that reg_mode should be checked whenever reg_offset is used as a 
replacement, so the first check is correct and the second should be changed.
Chung-Lin Tang - Sept. 15, 2010, 7:05 p.m.
On 2010/9/14 上午 02:13, Eric Botcazou wrote:
>> I think it's because the second else-if clause exchanges base_reg (which
>> is here known to be a constant) and XEXP(src,1), the latter becoming the
>> new "base reg" below. So reg_mode[REGNO(XEXP(src,1))] is what matters here.
>
> I think that reg_mode should be checked whenever reg_offset is used as a
> replacement, so the first check is correct and the second should be changed.
>

 From the surrounding context, I think reg_mode[] is checked in 
accordance with updating reg_base_reg[], not reg_offset[].

That said, this discussion is sort of diverging off my patch :)
My fix was only to add the required reg_symbol_ref[] tests, to ensure 
the case is 'offset' instead of 'sym + offset'. How the mode checks 
should be modified can be a later patch.
Eric Botcazou - Sept. 15, 2010, 9:18 p.m.
>  From the surrounding context, I think reg_mode[] is checked in
> accordance with updating reg_base_reg[], not reg_offset[].

No, I think the first check is for the first replacement, i.e. reg_offset.
The second check is a pasto and should be reg_mode[REGNO (base_reg)].

> That said, this discussion is sort of diverging off my patch :)

Not much, it's the same condition you're modifying so it's a good opportunity 
to fix everything instead of leaving another bug lurking.

Patch

Index: postreload.c
===================================================================
--- postreload.c	(revision 164232)
+++ postreload.c	(working copy)
@@ -2104,7 +2104,8 @@ 
 		       && (MODES_OK_FOR_MOVE2ADD
 			   (dst_mode, reg_mode[REGNO (XEXP (src, 1))])))
 		{
-		  if (reg_base_reg[REGNO (XEXP (src, 1))] < 0)
+		  if (reg_base_reg[REGNO (XEXP (src, 1))] < 0
+		      && reg_symbol_ref[REGNO (XEXP (src, 1))] == NULL_RTX)
 		    offset = reg_offset[REGNO (XEXP (src, 1))];
 		  /* Maybe the first register is known to be a
 		     constant.  */
@@ -2112,7 +2113,8 @@ 
 			   > move2add_last_label_luid
 			   && (MODES_OK_FOR_MOVE2ADD
 			       (dst_mode, reg_mode[REGNO (XEXP (src, 1))]))
-			   && reg_base_reg[REGNO (base_reg)] < 0)
+			   && reg_base_reg[REGNO (base_reg)] < 0
+			   && reg_symbol_ref[REGNO (base_reg)] == NULL_RTX)
 		    {
 		      offset = reg_offset[REGNO (base_reg)];
 		      base_reg = XEXP (src, 1);