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

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

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

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

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