Patchwork [ARM,RFC] Fix strange code in arm_legitimize_address

login
register
mail settings
Submitter Jie Zhang
Date Dec. 31, 2010, 2:26 p.m.
Message ID <4D1DE803.8060805@codesourcery.com>
Download mbox | patch
Permalink /patch/77082/
State New
Headers show

Comments

Jie Zhang - Dec. 31, 2010, 2:26 p.m.
Hi,

I found this while looking at something else. The following code in 
arm_legitimize_address is confusing for me:

   if (GET_CODE (x) == PLUS)
     {
       rtx xop0 = XEXP (x, 0);
       rtx xop1 = XEXP (x, 1);

       if (CONSTANT_P (xop0) && !symbol_mentioned_p (xop0))
	xop0 = force_reg (SImode, xop0);

       if (CONSTANT_P (xop1) && !symbol_mentioned_p (xop1))
	xop1 = force_reg (SImode, xop1);        <== code A

       if (ARM_BASE_REGISTER_RTX_P (xop0)
	  && GET_CODE (xop1) == CONST_INT)
	{
           ...                                   <== code B
         }
       ...
     }

The code B will never be executed since xop1 will never be a CONST_INT. 
If it were, it would have already been put into a reg by code A.

I did some digging. "!symbol_mentioned_p (xop1)" was brought into the 
code by revision 10681, which is every old and hence no patch email

The attached patch replaces !symbol_mentioned_p with symbol_mentioned_p 
in arm_legitimize_address. It shows an improvement for the following 
small test:

int buf[100000];
int foo ()
{
   return buf[40000];
}

For the default multilib and -O2, the trunk without the patch gives

foo:
	ldr	r3, .L2
	ldr	r2, .L2+4
	ldr	r0, [r2, r3]
	bx	lr
.L3:
	.align	2
.L2:
	.word	160000
	.word	buf
	.size	foo, .-foo
	.comm	buf,400000,4

With the patch:

foo:
	ldr	r3, .L2
	ldr	r0, [r3, #256]
	bx	lr
.L3:
	.align	2
.L2:
	.word	buf+159744
	.size	foo, .-foo
	.comm	buf,400000,4

So with the patch, we save one LDR instruction and one entry in constant 
pool.  No regressions are found when testing arm-none-eabi for the 
default multilib with --target_board=arm-sim.

OK?

Regards,
Jie Zhang - Jan. 17, 2011, 2:12 a.m.
PING.

On 12/31/2010 10:26 PM, Jie Zhang wrote:
> Hi,
>
> I found this while looking at something else. The following code in
> arm_legitimize_address is confusing for me:
>
> if (GET_CODE (x) == PLUS)
> {
> rtx xop0 = XEXP (x, 0);
> rtx xop1 = XEXP (x, 1);
>
> if (CONSTANT_P (xop0) && !symbol_mentioned_p (xop0))
> xop0 = force_reg (SImode, xop0);
>
> if (CONSTANT_P (xop1) && !symbol_mentioned_p (xop1))
> xop1 = force_reg (SImode, xop1); <== code A
>
> if (ARM_BASE_REGISTER_RTX_P (xop0)
> && GET_CODE (xop1) == CONST_INT)
> {
> ... <== code B
> }
> ...
> }
>
> The code B will never be executed since xop1 will never be a CONST_INT.
> If it were, it would have already been put into a reg by code A.
>
> I did some digging. "!symbol_mentioned_p (xop1)" was brought into the
> code by revision 10681, which is every old and hence no patch email
>
> The attached patch replaces !symbol_mentioned_p with symbol_mentioned_p
> in arm_legitimize_address. It shows an improvement for the following
> small test:
>
> int buf[100000];
> int foo ()
> {
> return buf[40000];
> }
>
> For the default multilib and -O2, the trunk without the patch gives
>
> foo:
> ldr r3, .L2
> ldr r2, .L2+4
> ldr r0, [r2, r3]
> bx lr
> .L3:
> .align 2
> .L2:
> .word 160000
> .word buf
> .size foo, .-foo
> .comm buf,400000,4
>
> With the patch:
>
> foo:
> ldr r3, .L2
> ldr r0, [r3, #256]
> bx lr
> .L3:
> .align 2
> .L2:
> .word buf+159744
> .size foo, .-foo
> .comm buf,400000,4
>
> So with the patch, we save one LDR instruction and one entry in constant
> pool. No regressions are found when testing arm-none-eabi for the
> default multilib with --target_board=arm-sim.
>
> OK?
>
> Regards,
Ramana Radhakrishnan - Jan. 20, 2011, 6:58 p.m.
On Mon, Jan 17, 2011 at 2:12 AM, Jie Zhang <jie@codesourcery.com> wrote:
> PING.
>
> On 12/31/2010 10:26 PM, Jie Zhang wrote:
>>
>> Hi,
>>
>> I found this while looking at something else. The following code in
>> arm_legitimize_address is confusing for me:
>>
>> if (GET_CODE (x) == PLUS)
>> {
>> rtx xop0 = XEXP (x, 0);
>> rtx xop1 = XEXP (x, 1);
>>
>> if (CONSTANT_P (xop0) && !symbol_mentioned_p (xop0))
>> xop0 = force_reg (SImode, xop0);
>>
>> if (CONSTANT_P (xop1) && !symbol_mentioned_p (xop1))
>> xop1 = force_reg (SImode, xop1); <== code A
>>
>> if (ARM_BASE_REGISTER_RTX_P (xop0)
>> && GET_CODE (xop1) == CONST_INT)
>> {
>> ... <== code B
>> }
>> ...
>> }
>>
>> The code B will never be executed since xop1 will never be a CONST_INT.
>> If it were, it would have already been put into a reg by code A.

Yeah I went through this and couldn't make out why this is the way it
is. Looks good to me though I can't approve or reject your patch.


cheers
Ramana
>
>> OK?
>>
>> Regards,
>
>
> --
> Jie Zhang
>
>
Jie Zhang - Jan. 21, 2011, 2:28 a.m.
On 01/21/2011 02:58 AM, Ramana Radhakrishnan wrote:
> On Mon, Jan 17, 2011 at 2:12 AM, Jie Zhang<jie@codesourcery.com>  wrote:
>> PING.
>>
>> On 12/31/2010 10:26 PM, Jie Zhang wrote:
>>>
>>> Hi,
>>>
>>> I found this while looking at something else. The following code in
>>> arm_legitimize_address is confusing for me:
>>>
>>> if (GET_CODE (x) == PLUS)
>>> {
>>> rtx xop0 = XEXP (x, 0);
>>> rtx xop1 = XEXP (x, 1);
>>>
>>> if (CONSTANT_P (xop0)&&  !symbol_mentioned_p (xop0))
>>> xop0 = force_reg (SImode, xop0);
>>>
>>> if (CONSTANT_P (xop1)&&  !symbol_mentioned_p (xop1))
>>> xop1 = force_reg (SImode, xop1);<== code A
>>>
>>> if (ARM_BASE_REGISTER_RTX_P (xop0)
>>> &&  GET_CODE (xop1) == CONST_INT)
>>> {
>>> ...<== code B
>>> }
>>> ...
>>> }
>>>
>>> The code B will never be executed since xop1 will never be a CONST_INT.
>>> If it were, it would have already been put into a reg by code A.
>
> Yeah I went through this and couldn't make out why this is the way it
> is. Looks good to me though I can't approve or reject your patch.
>
Thanks for taking a look! That piece of code can be traced back to more 
than 15 years ago. Maybe only Richard still remembers the original 
reason behind it.


Regards,
Ramana Radhakrishnan - Jan. 21, 2011, 10:35 a.m.
On Fri, 2011-01-21 at 10:28 +0800, Jie Zhang wrote:
> On 01/21/2011 02:58 AM, Ramana Radhakrishnan wrote:
> > On Mon, Jan 17, 2011 at 2:12 AM, Jie Zhang<jie@codesourcery.com>  wrote:
> >> PING.
> >>
> >> On 12/31/2010 10:26 PM, Jie Zhang wrote:
> >>>
> >>> Hi,
> >>>
> >>> I found this while looking at something else. The following code in
> >>> arm_legitimize_address is confusing for me:
> >>>
> >>> if (GET_CODE (x) == PLUS)
> >>> {
> >>> rtx xop0 = XEXP (x, 0);
> >>> rtx xop1 = XEXP (x, 1);
> >>>
> >>> if (CONSTANT_P (xop0)&&  !symbol_mentioned_p (xop0))
> >>> xop0 = force_reg (SImode, xop0);
> >>>
> >>> if (CONSTANT_P (xop1)&&  !symbol_mentioned_p (xop1))
> >>> xop1 = force_reg (SImode, xop1);<== code A
> >>>
> >>> if (ARM_BASE_REGISTER_RTX_P (xop0)
> >>> &&  GET_CODE (xop1) == CONST_INT)
> >>> {
> >>> ...<== code B
> >>> }
> >>> ...
> >>> }
> >>>
> >>> The code B will never be executed since xop1 will never be a CONST_INT.
> >>> If it were, it would have already been put into a reg by code A.
> >
> > Yeah I went through this and couldn't make out why this is the way it
> > is. Looks good to me though I can't approve or reject your patch.
> >
> Thanks for taking a look! That piece of code can be traced back to more 
> than 15 years ago. Maybe only Richard still remembers the original 
> reason behind it.

Probably - Just noticed that you'd tested only for the basic
multilibs.It might be worth testing for a vfp variant at v7-a just of
paranoia since this is a code path that hasn't been exercised for 15
years though I think it should be safe.

Ramana
> 
> 
> Regards,
Jie Zhang - Jan. 24, 2011, 4:31 p.m.
On 01/21/2011 06:35 PM, Ramana Radhakrishnan wrote:
>
> On Fri, 2011-01-21 at 10:28 +0800, Jie Zhang wrote:
>> On 01/21/2011 02:58 AM, Ramana Radhakrishnan wrote:
>>> On Mon, Jan 17, 2011 at 2:12 AM, Jie Zhang<jie@codesourcery.com>   wrote:
>>>> PING.
>>>>
>>>> On 12/31/2010 10:26 PM, Jie Zhang wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> I found this while looking at something else. The following code in
>>>>> arm_legitimize_address is confusing for me:
>>>>>
>>>>> if (GET_CODE (x) == PLUS)
>>>>> {
>>>>> rtx xop0 = XEXP (x, 0);
>>>>> rtx xop1 = XEXP (x, 1);
>>>>>
>>>>> if (CONSTANT_P (xop0)&&   !symbol_mentioned_p (xop0))
>>>>> xop0 = force_reg (SImode, xop0);
>>>>>
>>>>> if (CONSTANT_P (xop1)&&   !symbol_mentioned_p (xop1))
>>>>> xop1 = force_reg (SImode, xop1);<== code A
>>>>>
>>>>> if (ARM_BASE_REGISTER_RTX_P (xop0)
>>>>> &&   GET_CODE (xop1) == CONST_INT)
>>>>> {
>>>>> ...<== code B
>>>>> }
>>>>> ...
>>>>> }
>>>>>
>>>>> The code B will never be executed since xop1 will never be a CONST_INT.
>>>>> If it were, it would have already been put into a reg by code A.
>>>
>>> Yeah I went through this and couldn't make out why this is the way it
>>> is. Looks good to me though I can't approve or reject your patch.
>>>
>> Thanks for taking a look! That piece of code can be traced back to more
>> than 15 years ago. Maybe only Richard still remembers the original
>> reason behind it.
>
> Probably - Just noticed that you'd tested only for the basic
> multilibs.It might be worth testing for a vfp variant at v7-a just of
> paranoia since this is a code path that hasn't been exercised for 15
> years though I think it should be safe.
>
I did a regression testing with "-march=armv7-a -mfloat-abi=softfp 
-mfpu=neon" on qemu for gcc, g++, libstdc++. No regressions are found.

Patch


	* config/arm/arm.c (arm_legitimize_address): Replace
	!symbol_mentioned_p with symbol_mentioned_p.

Index: config/arm/arm.c
===================================================================
--- config/arm/arm.c	(revision 168310)
+++ config/arm/arm.c	(working copy)
@@ -6217,10 +6217,10 @@  arm_legitimize_address (rtx x, rtx orig_
       rtx xop0 = XEXP (x, 0);
       rtx xop1 = XEXP (x, 1);
 
-      if (CONSTANT_P (xop0) && !symbol_mentioned_p (xop0))
+      if (CONSTANT_P (xop0) && symbol_mentioned_p (xop0))
 	xop0 = force_reg (SImode, xop0);
 
-      if (CONSTANT_P (xop1) && !symbol_mentioned_p (xop1))
+      if (CONSTANT_P (xop1) && symbol_mentioned_p (xop1))
 	xop1 = force_reg (SImode, xop1);
 
       if (ARM_BASE_REGISTER_RTX_P (xop0)
@@ -6269,7 +6269,7 @@  arm_legitimize_address (rtx x, rtx orig_
       if (CONSTANT_P (xop0))
 	xop0 = force_reg (SImode, xop0);
 
-      if (CONSTANT_P (xop1) && ! symbol_mentioned_p (xop1))
+      if (CONSTANT_P (xop1) && symbol_mentioned_p (xop1))
 	xop1 = force_reg (SImode, xop1);
 
       if (xop0 != XEXP (x, 0) || xop1 != XEXP (x, 1))