[patch, ARM] Fix PR42017, LR not used in leaf functions

[patch, ARM] Fix PR42017, LR not used in leaf functions

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 1579 bytes --]

Hi, this patch tries to solve the problem of the LR register not
being used in leaf functions on ARM.

Looking at the dumps, it shows that register 14 (lr) conflicts with all
allocnos throughout the entire leaf procedure. A little digging shows
that lr is present in the OBJECT_CONFLICT_HARD_REGS() set of all
allocnos, which suggests that this may manifest from a convention of
some sort the code is currently following.

It turns out that, during the IRA liveness computations, the DF initing
of live hard regs, starting from the bottom end of the function, adds
EPILOGUE_USES right from the start. With no call sites to kill its
liveness, the entire procedure is prohibited from using LR at all.

This problem may also be more serious than just leaf functions.
Theoretically, this may affect all allocnos that happen to completely
lie on paths that reach the end of function without a call site, even in
non-leaf functions. All these are deprived of LR as an usable register.

My fix here simply adds 'reload_completed' as an additional condition
for EPILOGUE_USES to return true for LR_REGNUM. I think this should be
valid, as correct LR save/restoring is handled by the epilogue/prologue
code; it should be safe for IRA to treat it as a normal call-used register.

I did a cross-test on QEMU with clean results, plus a successful native
bootstrap on a Pandaboard. Is this okay for trunk?

Thanks,
Chung-Lin

2011-04-28  Chung-Lin Tang  <cltang@codesourcery.com>

	PR target/42017
	* config/arm/arm.h (EPILOGUE_USES): Only return true
	for LR_REGNUM after reload_completed.


[-- Attachment #2: arm-epilogue_uses.diff --]
[-- Type: text/plain, Size: 491 bytes --]

Index: config/arm/arm.h
===================================================================
--- config/arm/arm.h	(revision 173046)
+++ config/arm/arm.h	(working copy)
@@ -1627,7 +1627,7 @@
    frame.  */
 #define EXIT_IGNORE_STACK 1
 
-#define EPILOGUE_USES(REGNO) ((REGNO) == LR_REGNUM)
+#define EPILOGUE_USES(REGNO) (reload_completed && (REGNO) == LR_REGNUM)
 
 /* Determine if the epilogue should be output as RTL.
    You should override this if you define FUNCTION_EXTRA_EPILOGUE.  */

Re: [patch, ARM] Fix PR42017, LR not used in leaf functions

[Richard Sandiford]

Chung-Lin Tang <cltang@codesourcery.com> writes:
> My fix here simply adds 'reload_completed' as an additional condition
> for EPILOGUE_USES to return true for LR_REGNUM. I think this should be
> valid, as correct LR save/restoring is handled by the epilogue/prologue
> code; it should be safe for IRA to treat it as a normal call-used register.

FWIW, epilogue_completed might be a more accurate choice.

It seems a lot of other ports suffer from the same problem though.
I wonder which targets really do want to make a register live throughout
the function?  If none do, perhaps we should say that this macro is
only meaningful once the epilogue has been generated.

Richard

Re: [patch, ARM] Fix PR42017, LR not used in leaf functions

[Richard Sandiford]

Richard Sandiford <richard.sandiford@linaro.org> writes:
> Chung-Lin Tang <cltang@codesourcery.com> writes:
>> My fix here simply adds 'reload_completed' as an additional condition
>> for EPILOGUE_USES to return true for LR_REGNUM. I think this should be
>> valid, as correct LR save/restoring is handled by the epilogue/prologue
>> code; it should be safe for IRA to treat it as a normal call-used register.
>
> FWIW, epilogue_completed might be a more accurate choice.

I still stand by this, although I realise no other target does it.

> It seems a lot of other ports suffer from the same problem though.
> I wonder which targets really do want to make a register live throughout
> the function?  If none do, perhaps we should say that this macro is
> only meaningful once the epilogue has been generated.

To answer my own question, I suppose VRSAVE is one.  So I was wrong
about the target-independent "fix".

Richard

Re: [patch, ARM] Fix PR42017, LR not used in leaf functions

[Chung-Lin Tang]

[-- Attachment #1: Type: text/plain, Size: 1472 bytes --]

On 2011/5/13 04:26 PM, Richard Sandiford wrote:
> Richard Sandiford <richard.sandiford@linaro.org> writes:
>> Chung-Lin Tang <cltang@codesourcery.com> writes:
>>> My fix here simply adds 'reload_completed' as an additional condition
>>> for EPILOGUE_USES to return true for LR_REGNUM. I think this should be
>>> valid, as correct LR save/restoring is handled by the epilogue/prologue
>>> code; it should be safe for IRA to treat it as a normal call-used register.
>>
>> FWIW, epilogue_completed might be a more accurate choice.
> 
> I still stand by this, although I realise no other target does it.

Did a re-test of the patch just to be sure, as expected the test results
were also clear. Attached is the updated patch.

>> It seems a lot of other ports suffer from the same problem though.
>> I wonder which targets really do want to make a register live throughout
>> the function?  If none do, perhaps we should say that this macro is
>> only meaningful once the epilogue has been generated.
> 
> To answer my own question, I suppose VRSAVE is one.  So I was wrong
> about the target-independent "fix".
> 
> Richard

To rehash what I remember we discussed at LDS, such registers like
VRSAVE might be more appropriately placed in global regs. It looks like
EPILOGUE_USES could be more clarified in its use...

To Richard Earnshaw and Ramana, is the patch okay for trunk? This should
be a not-so-insignificant performance regression-fix/improvement.

Thanks,
Chung-Lin

[-- Attachment #2: arm-epilogue_uses-2.diff --]
[-- Type: text/plain, Size: 492 bytes --]

Index: config/arm/arm.h
===================================================================
--- config/arm/arm.h	(revision 173814)
+++ config/arm/arm.h	(working copy)
@@ -1627,7 +1627,7 @@
    frame.  */
 #define EXIT_IGNORE_STACK 1
 
-#define EPILOGUE_USES(REGNO) ((REGNO) == LR_REGNUM)
+#define EPILOGUE_USES(REGNO) (epilogue_completed && (REGNO) == LR_REGNUM)
 
 /* Determine if the epilogue should be output as RTL.
    You should override this if you define FUNCTION_EXTRA_EPILOGUE.  */

Re: [patch, ARM] Fix PR42017, LR not used in leaf functions

[Eric Botcazou]

> > It seems a lot of other ports suffer from the same problem though.
> > I wonder which targets really do want to make a register live throughout
> > the function?  If none do, perhaps we should say that this macro is
> > only meaningful once the epilogue has been generated.
>
> To answer my own question, I suppose VRSAVE is one.  So I was wrong
> about the target-independent "fix".

We have %i7 on the SPARC (register where the return address register is saved).

-- 
Eric Botcazou

Re: [patch, ARM] Fix PR42017, LR not used in leaf functions

[Ramana Radhakrishnan]

On 17/05/11 14:10, Chung-Lin Tang wrote:
> On 2011/5/13 04:26 PM, Richard Sandiford wrote:
>> Richard Sandiford<richard.sandiford@linaro.org>  writes:
>>> Chung-Lin Tang<cltang@codesourcery.com>  writes:
>>>> My fix here simply adds 'reload_completed' as an additional condition
>>>> for EPILOGUE_USES to return true for LR_REGNUM. I think this should be
>>>> valid, as correct LR save/restoring is handled by the epilogue/prologue
>>>> code; it should be safe for IRA to treat it as a normal call-used register.
>>>
>>> FWIW, epilogue_completed might be a more accurate choice.
>>
>> I still stand by this, although I realise no other target does it.
>
> Did a re-test of the patch just to be sure, as expected the test results
> were also clear. Attached is the updated patch.

Can you specify what you tested with this patch ?

So, it's interesting to note that the use of this was changed in 2007 by 
zadeck as a part of the df merge.

I can't find the patch trail beyond this on the lists.

http://gcc.gnu.org/viewvc/branches/dataflow-branch/gcc/config/arm/arm.h?r1=120281&r2=121501

It might be better to understand why this was done in the first place 
for the ARM port as part of the Dataflow bring up and why folks wanted 
to make this unconditional.

cheers
Ramana

Re: [patch, ARM] Fix PR42017, LR not used in leaf functions

[Chung-Lin Tang]

On 2011/5/20 下午 07:41, Ramana Radhakrishnan wrote:
> On 17/05/11 14:10, Chung-Lin Tang wrote:
>> On 2011/5/13 04:26 PM, Richard Sandiford wrote:
>>> Richard Sandiford<richard.sandiford@linaro.org>  writes:
>>>> Chung-Lin Tang<cltang@codesourcery.com>  writes:
>>>>> My fix here simply adds 'reload_completed' as an additional condition
>>>>> for EPILOGUE_USES to return true for LR_REGNUM. I think this should be
>>>>> valid, as correct LR save/restoring is handled by the
>>>>> epilogue/prologue
>>>>> code; it should be safe for IRA to treat it as a normal call-used
>>>>> register.
>>>>
>>>> FWIW, epilogue_completed might be a more accurate choice.
>>>
>>> I still stand by this, although I realise no other target does it.
>>
>> Did a re-test of the patch just to be sure, as expected the test results
>> were also clear. Attached is the updated patch.
> 
> Can you specify what you tested with this patch ?

Native bootstrap success, plus C/C++ and libstdc++ tests. IIRC I also
saw one or two FAIL->PASS in the results too (forgot specific testcases)

> 
> So, it's interesting to note that the use of this was changed in 2007 by
> zadeck as a part of the df merge.
> 
> I can't find the patch trail beyond this on the lists.
> 
> http://gcc.gnu.org/viewvc/branches/dataflow-branch/gcc/config/arm/arm.h?r1=120281&r2=121501
> 
> 
> It might be better to understand why this was done in the first place
> for the ARM port as part of the Dataflow bring up and why folks wanted
> to make this unconditional.

Oh dear, more archeology...

Thanks,
Chung-Lin

Re: [patch, ARM] Fix PR42017, LR not used in leaf functions

[Chung-Lin Tang]

On 2011/5/20 07:46 PM, Chung-Lin Tang wrote:
> On 2011/5/20 下午 07:41, Ramana Radhakrishnan wrote:
>> On 17/05/11 14:10, Chung-Lin Tang wrote:
>>> On 2011/5/13 04:26 PM, Richard Sandiford wrote:
>>>> Richard Sandiford<richard.sandiford@linaro.org>  writes:
>>>>> Chung-Lin Tang<cltang@codesourcery.com>  writes:
>>>>>> My fix here simply adds 'reload_completed' as an additional condition
>>>>>> for EPILOGUE_USES to return true for LR_REGNUM. I think this should be
>>>>>> valid, as correct LR save/restoring is handled by the
>>>>>> epilogue/prologue
>>>>>> code; it should be safe for IRA to treat it as a normal call-used
>>>>>> register.
>>>>>
>>>>> FWIW, epilogue_completed might be a more accurate choice.
>>>>
>>>> I still stand by this, although I realise no other target does it.
>>>
>>> Did a re-test of the patch just to be sure, as expected the test results
>>> were also clear. Attached is the updated patch.
>>
>> Can you specify what you tested with this patch ?
> 
> Native bootstrap success, plus C/C++ and libstdc++ tests. IIRC I also
> saw one or two FAIL->PASS in the results too (forgot specific testcases)
> 
>>
>> So, it's interesting to note that the use of this was changed in 2007 by
>> zadeck as a part of the df merge.
>>
>> I can't find the patch trail beyond this on the lists.
>>
>> http://gcc.gnu.org/viewvc/branches/dataflow-branch/gcc/config/arm/arm.h?r1=120281&r2=121501
>>
>>
>> It might be better to understand why this was done in the first place
>> for the ARM port as part of the Dataflow bring up and why folks wanted
>> to make this unconditional.

Digging through the repository, this is my explanation, FWIW:

1) The gen_prologue_uses() of LR were added back in Dec.2000 (r38467),
when "ce2" was still the if-convert-after-reload pass, placed after
prologue-epilogue construction. (hence the arm_expand_prologue() comment
about preventing ce2 using LR)

2) if-conversion after combine was added in Oct.2002 (r58547), which
became the new "ce2" (pre-reload); ifcvt after reload became "ce3". The
comments in arm_expand_prologue() were not updated.

3) dataflow-branch work was circa 2007. RTL-ifcvt seemed to be updated
during this time, hence removal of the LR-uses in arm_expand_prologue()
seems reasonable. My guess here: ce2 was mistaken to be
ifcvt-after-combine (rather than the originally intended
ifcvt-after-reload, now ce3) by the comments; considering the
arm_expand_prologue() bits were updated, the comments may have been read
seriously.

4) Since "ce2" was a pre-reload pass by then, the unconditionalizing of
EPILOGUE_USES was probably intended to be a supplemental change, to
support removing those gen_prologue_use()s.

I hope this is a reasonable explanation, but do note a lot of this is
guessing :)

I tried taking the last version of the dataflow-branch (circa 4.3) and
did cross-test run compares of EPILOGUE_USES with and without the
reload_completed conditionalization. The C testsuite results were clean.

The LR-not-used symptoms seem triggered by this EPILOGUE_USES change
since then. As the PR42017 submitter lists the affected GCC versions,
this regression has been present since post-4.2.

Given the above explanation, and considering that the tests on current
trunk are okay, plus we're in stage1 right now, is this
re-conditionalizing EPILOGUE_USES change okay to commit?

Thanks,
Chung-Lin

Ping Re: [patch, ARM] Fix PR42017, LR not used in leaf functions

[Chung-Lin Tang]

Ping.
On 2011/5/26 01:29 AM, Chung-Lin Tang wrote:
> On 2011/5/20 07:46 PM, Chung-Lin Tang wrote:
>> On 2011/5/20 下午 07:41, Ramana Radhakrishnan wrote:
>>> On 17/05/11 14:10, Chung-Lin Tang wrote:
>>>> On 2011/5/13 04:26 PM, Richard Sandiford wrote:
>>>>> Richard Sandiford<richard.sandiford@linaro.org>  writes:
>>>>>> Chung-Lin Tang<cltang@codesourcery.com>  writes:
>>>>>>> My fix here simply adds 'reload_completed' as an additional condition
>>>>>>> for EPILOGUE_USES to return true for LR_REGNUM. I think this should be
>>>>>>> valid, as correct LR save/restoring is handled by the
>>>>>>> epilogue/prologue
>>>>>>> code; it should be safe for IRA to treat it as a normal call-used
>>>>>>> register.
>>>>>>
>>>>>> FWIW, epilogue_completed might be a more accurate choice.
>>>>>
>>>>> I still stand by this, although I realise no other target does it.
>>>>
>>>> Did a re-test of the patch just to be sure, as expected the test results
>>>> were also clear. Attached is the updated patch.
>>>
>>> Can you specify what you tested with this patch ?
>>
>> Native bootstrap success, plus C/C++ and libstdc++ tests. IIRC I also
>> saw one or two FAIL->PASS in the results too (forgot specific testcases)
>>
>>>
>>> So, it's interesting to note that the use of this was changed in 2007 by
>>> zadeck as a part of the df merge.
>>>
>>> I can't find the patch trail beyond this on the lists.
>>>
>>> http://gcc.gnu.org/viewvc/branches/dataflow-branch/gcc/config/arm/arm.h?r1=120281&r2=121501
>>>
>>>
>>> It might be better to understand why this was done in the first place
>>> for the ARM port as part of the Dataflow bring up and why folks wanted
>>> to make this unconditional.
> 
> Digging through the repository, this is my explanation, FWIW:
> 
> 1) The gen_prologue_uses() of LR were added back in Dec.2000 (r38467),
> when "ce2" was still the if-convert-after-reload pass, placed after
> prologue-epilogue construction. (hence the arm_expand_prologue() comment
> about preventing ce2 using LR)
> 
> 2) if-conversion after combine was added in Oct.2002 (r58547), which
> became the new "ce2" (pre-reload); ifcvt after reload became "ce3". The
> comments in arm_expand_prologue() were not updated.
> 
> 3) dataflow-branch work was circa 2007. RTL-ifcvt seemed to be updated
> during this time, hence removal of the LR-uses in arm_expand_prologue()
> seems reasonable. My guess here: ce2 was mistaken to be
> ifcvt-after-combine (rather than the originally intended
> ifcvt-after-reload, now ce3) by the comments; considering the
> arm_expand_prologue() bits were updated, the comments may have been read
> seriously.
> 
> 4) Since "ce2" was a pre-reload pass by then, the unconditionalizing of
> EPILOGUE_USES was probably intended to be a supplemental change, to
> support removing those gen_prologue_use()s.
> 
> I hope this is a reasonable explanation, but do note a lot of this is
> guessing :)
> 
> I tried taking the last version of the dataflow-branch (circa 4.3) and
> did cross-test run compares of EPILOGUE_USES with and without the
> reload_completed conditionalization. The C testsuite results were clean.
> 
> The LR-not-used symptoms seem triggered by this EPILOGUE_USES change
> since then. As the PR42017 submitter lists the affected GCC versions,
> this regression has been present since post-4.2.
> 
> Given the above explanation, and considering that the tests on current
> trunk are okay, plus we're in stage1 right now, is this
> re-conditionalizing EPILOGUE_USES change okay to commit?
> 
> Thanks,
> Chung-Lin