diff mbox

[ARM,RFC] Fix strange code in arm_legitimize_address

Message ID 4D1DE803.8060805@codesourcery.com
State New
Headers show

Commit Message

Jie Zhang Dec. 31, 2010, 2:26 p.m. UTC 
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,

Comments

Jie Zhang Jan. 17, 2011, 2:12 a.m. UTC | #1 
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. UTC | #2 
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. UTC | #3 
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. UTC | #4 
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. UTC | #5 
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.
diff mbox

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))