* [ARM, RFC] Fix strange code in arm_legitimize_address
@ 2010-12-31 15:49 Jie Zhang
2011-01-17 2:39 ` PING " Jie Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Jie Zhang @ 2010-12-31 15:49 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 1655 bytes --]
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
[-- Attachment #2: gcc-arm-fix-legitimize-address.diff --]
[-- Type: text/x-diff, Size: 1094 bytes --]
* 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))
^ permalink raw reply [flat|nested] 6+ messages in thread
* PING [ARM, RFC] Fix strange code in arm_legitimize_address
2010-12-31 15:49 [ARM, RFC] Fix strange code in arm_legitimize_address Jie Zhang
@ 2011-01-17 2:39 ` Jie Zhang
2011-01-20 19:38 ` Ramana Radhakrishnan
0 siblings, 1 reply; 6+ messages in thread
From: Jie Zhang @ 2011-01-17 2:39 UTC (permalink / raw)
To: gcc-patches, Richard Earnshaw
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,
--
Jie Zhang
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PING [ARM, RFC] Fix strange code in arm_legitimize_address
2011-01-17 2:39 ` PING " Jie Zhang
@ 2011-01-20 19:38 ` Ramana Radhakrishnan
2011-01-21 4:54 ` Jie Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Ramana Radhakrishnan @ 2011-01-20 19:38 UTC (permalink / raw)
To: Jie Zhang; +Cc: gcc-patches, Richard Earnshaw
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
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PING [ARM, RFC] Fix strange code in arm_legitimize_address
2011-01-20 19:38 ` Ramana Radhakrishnan
@ 2011-01-21 4:54 ` Jie Zhang
2011-01-21 10:43 ` Ramana Radhakrishnan
0 siblings, 1 reply; 6+ messages in thread
From: Jie Zhang @ 2011-01-21 4:54 UTC (permalink / raw)
To: Ramana Radhakrishnan; +Cc: gcc-patches, Richard Earnshaw
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,
--
Jie Zhang
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PING [ARM, RFC] Fix strange code in arm_legitimize_address
2011-01-21 4:54 ` Jie Zhang
@ 2011-01-21 10:43 ` Ramana Radhakrishnan
2011-01-24 16:59 ` Jie Zhang
0 siblings, 1 reply; 6+ messages in thread
From: Ramana Radhakrishnan @ 2011-01-21 10:43 UTC (permalink / raw)
To: Jie Zhang; +Cc: Ramana Radhakrishnan, gcc-patches, Richard Earnshaw
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,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PING [ARM, RFC] Fix strange code in arm_legitimize_address
2011-01-21 10:43 ` Ramana Radhakrishnan
@ 2011-01-24 16:59 ` Jie Zhang
0 siblings, 0 replies; 6+ messages in thread
From: Jie Zhang @ 2011-01-24 16:59 UTC (permalink / raw)
To: ramana.radhakrishnan; +Cc: Ramana Radhakrishnan, gcc-patches, Richard Earnshaw
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.
--
Jie Zhang
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-01-24 16:31 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-12-31 15:49 [ARM, RFC] Fix strange code in arm_legitimize_address Jie Zhang
2011-01-17 2:39 ` PING " Jie Zhang
2011-01-20 19:38 ` Ramana Radhakrishnan
2011-01-21 4:54 ` Jie Zhang
2011-01-21 10:43 ` Ramana Radhakrishnan
2011-01-24 16:59 ` Jie Zhang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).