Patchwork [ARM,LRA] Fixed bootstrap failure in Thumb mode

login
register
mail settings
Submitter Yvan Roux
Date Nov. 7, 2013, 2:56 p.m.
Message ID <CAD57uCejV6qjR2uvwP4cMfBx-qaxZHH9+reqhW91QO2-Ry7Rvg@mail.gmail.com>
Download mbox | patch
Permalink /patch/289375/
State New
Headers show

Comments

Yvan Roux - Nov. 7, 2013, 2:56 p.m.
Hi,

this patch fixed an LRA cycling due to secondary reload (Thumb mode).
Notice that this patch is a prerequisite to turn on LRA by default on
ARM.  Bootstrapped on a9 and a15 without any regression in the
testsuite as LRA is off by default and with the regression reported in
the thread bellow when LRA is on.

http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00725.html

Thanks,
Yvan

2013-11-07  Yvan Roux  <yvan.roux@linaro.org>

        * config/arm/arm.h (THUMB_SECONDARY_INPUT_RELOAD_CLASS): Return NO_REGS
        for LRA.
Yvan Roux - Nov. 18, 2013, 8:40 a.m.
Ping.

On 7 November 2013 15:56, Yvan Roux <yvan.roux@linaro.org> wrote:
> Hi,
>
> this patch fixed an LRA cycling due to secondary reload (Thumb mode).
> Notice that this patch is a prerequisite to turn on LRA by default on
> ARM.  Bootstrapped on a9 and a15 without any regression in the
> testsuite as LRA is off by default and with the regression reported in
> the thread bellow when LRA is on.
>
> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00725.html
>
> Thanks,
> Yvan
>
> 2013-11-07  Yvan Roux  <yvan.roux@linaro.org>
>
>         * config/arm/arm.h (THUMB_SECONDARY_INPUT_RELOAD_CLASS): Return NO_REGS
>         for LRA.
Yvan Roux - Nov. 27, 2013, 10:18 a.m.
Ping

On 18 November 2013 09:40, Yvan Roux <yvan.roux@linaro.org> wrote:
> Ping.
>
> On 7 November 2013 15:56, Yvan Roux <yvan.roux@linaro.org> wrote:
>> Hi,
>>
>> this patch fixed an LRA cycling due to secondary reload (Thumb mode).
>> Notice that this patch is a prerequisite to turn on LRA by default on
>> ARM.  Bootstrapped on a9 and a15 without any regression in the
>> testsuite as LRA is off by default and with the regression reported in
>> the thread bellow when LRA is on.
>>
>> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00725.html
>>
>> Thanks,
>> Yvan
>>
>> 2013-11-07  Yvan Roux  <yvan.roux@linaro.org>
>>
>>         * config/arm/arm.h (THUMB_SECONDARY_INPUT_RELOAD_CLASS): Return NO_REGS
>>         for LRA.
Jeff Law - Nov. 27, 2013, 5:16 p.m.
On 11/27/13 03:18, Yvan Roux wrote:
> Ping
>
> On 18 November 2013 09:40, Yvan Roux <yvan.roux@linaro.org> wrote:
>> Ping.
>>
>> On 7 November 2013 15:56, Yvan Roux <yvan.roux@linaro.org> wrote:
>>> Hi,
>>>
>>> this patch fixed an LRA cycling due to secondary reload (Thumb mode).
>>> Notice that this patch is a prerequisite to turn on LRA by default on
>>> ARM.  Bootstrapped on a9 and a15 without any regression in the
>>> testsuite as LRA is off by default and with the regression reported in
>>> the thread bellow when LRA is on.
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00725.html
>>>
>>> Thanks,
>>> Yvan
>>>
>>> 2013-11-07  Yvan Roux  <yvan.roux@linaro.org>
>>>
>>>          * config/arm/arm.h (THUMB_SECONDARY_INPUT_RELOAD_CLASS): Return NO_REGS
>>>          for LRA
?

How can that be correct?

The secondary reload macros/hooks define cases where additional 
registers are needed to reload certain forms of rtl.  I doubt the use of 
LRA completely eliminates the need for secondary reloads.

Jeff
Yvan Roux - Nov. 27, 2013, 5:49 p.m.
> How can that be correct?
>
> The secondary reload macros/hooks define cases where additional registers
> are needed to reload certain forms of rtl.  I doubt the use of LRA
> completely eliminates the need for secondary reloads.

Vladimir explained me that in that case on arm, secondary reload hook
confuses LRA, and that returning NO_REGS will let LRA deal with
constraints, but I may have badly understand what he said.

Yvan
Richard Earnshaw - Nov. 27, 2013, 5:52 p.m.
On 27/11/13 17:49, Yvan Roux wrote:
>> How can that be correct?
>>
>> The secondary reload macros/hooks define cases where additional registers
>> are needed to reload certain forms of rtl.  I doubt the use of LRA
>> completely eliminates the need for secondary reloads.
> 
> Vladimir explained me that in that case on arm, secondary reload hook
> confuses LRA, and that returning NO_REGS will let LRA deal with
> constraints, but I may have badly understand what he said.
> 
> Yvan
> 

But wasn't that in the context of PREFERRED_RELOAD_CLASS?  That's a
different beast.

R.
Vladimir Makarov - Nov. 27, 2013, 5:58 p.m.
On 11/27/2013, 12:16 PM, Jeff Law wrote:
> On 11/27/13 03:18, Yvan Roux wrote:
>> Ping
>>
>> On 18 November 2013 09:40, Yvan Roux <yvan.roux@linaro.org> wrote:
>>> Ping.
>>>
>>> On 7 November 2013 15:56, Yvan Roux <yvan.roux@linaro.org> wrote:
>>>> Hi,
>>>>
>>>> this patch fixed an LRA cycling due to secondary reload (Thumb mode).
>>>> Notice that this patch is a prerequisite to turn on LRA by default on
>>>> ARM.  Bootstrapped on a9 and a15 without any regression in the
>>>> testsuite as LRA is off by default and with the regression reported in
>>>> the thread bellow when LRA is on.
>>>>
>>>> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00725.html
>>>>
>>>> Thanks,
>>>> Yvan
>>>>
>>>> 2013-11-07  Yvan Roux  <yvan.roux@linaro.org>
>>>>
>>>>          * config/arm/arm.h (THUMB_SECONDARY_INPUT_RELOAD_CLASS): 
>>>> Return NO_REGS
>>>>          for LRA
> ?
>
> How can that be correct?
>
> The secondary reload macros/hooks define cases where additional 
> registers are needed to reload certain forms of rtl.  I doubt the use 
> of LRA completely eliminates the need for secondary reloads.
   When I designed LRA I wanted to remove as many hooks as possible 
because I thought insn constraints as major info source are enough. 
Unfortunately I did not succeed fully with secondary reload hooks and 
macros.  It is still needed for some complicated cases but general rule 
to port LRA to a target is to try to switch these hooks off.

   For example, LRA can generate a move of pseudos of classes for which 
hardware has no real insn.  After that looking on the move constraints,  
LRA can generate more move insns and additional pseudos to generate 
moves (loads or stores if additional pseudos got NO_REGS) which 
represent real hardware insns.  In complicated cases when these macros 
are still needed, LRA runs into infinite loop of such move generation if 
the macros are not used.

   I might be return to a project to remove necessity of such hooks and 
macros for LRA but I am not sure when.  I guess I need to write a small 
guidance too how to port LRA.

   Also I found that in many cases as here, the macros although working 
for reload can confuse LRA (because of different reload pass and LRA 
implementation approaches).  So I guess the patch is ok although I can 
not approve it as I am not an arm port maintainer.
Yvan Roux - Nov. 27, 2013, 6:07 p.m.
On 27 November 2013 18:58, Vladimir Makarov <vmakarov@redhat.com> wrote:
> On 11/27/2013, 12:16 PM, Jeff Law wrote:
>>
>> On 11/27/13 03:18, Yvan Roux wrote:
>>>
>>> Ping
>>>
>>> On 18 November 2013 09:40, Yvan Roux <yvan.roux@linaro.org> wrote:
>>>>
>>>> Ping.
>>>>
>>>> On 7 November 2013 15:56, Yvan Roux <yvan.roux@linaro.org> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> this patch fixed an LRA cycling due to secondary reload (Thumb mode).
>>>>> Notice that this patch is a prerequisite to turn on LRA by default on
>>>>> ARM.  Bootstrapped on a9 and a15 without any regression in the
>>>>> testsuite as LRA is off by default and with the regression reported in
>>>>> the thread bellow when LRA is on.
>>>>>
>>>>> http://gcc.gnu.org/ml/gcc-patches/2013-11/msg00725.html
>>>>>
>>>>> Thanks,
>>>>> Yvan
>>>>>
>>>>> 2013-11-07  Yvan Roux  <yvan.roux@linaro.org>
>>>>>
>>>>>          * config/arm/arm.h (THUMB_SECONDARY_INPUT_RELOAD_CLASS):
>>>>> Return NO_REGS
>>>>>          for LRA
>>
>> ?
>>
>> How can that be correct?
>>
>> The secondary reload macros/hooks define cases where additional registers
>> are needed to reload certain forms of rtl.  I doubt the use of LRA
>> completely eliminates the need for secondary reloads.
>
>   When I designed LRA I wanted to remove as many hooks as possible because I
> thought insn constraints as major info source are enough. Unfortunately I
> did not succeed fully with secondary reload hooks and macros.  It is still
> needed for some complicated cases but general rule to port LRA to a target
> is to try to switch these hooks off.
>
>   For example, LRA can generate a move of pseudos of classes for which
> hardware has no real insn.  After that looking on the move constraints,  LRA
> can generate more move insns and additional pseudos to generate moves (loads
> or stores if additional pseudos got NO_REGS) which represent real hardware
> insns.  In complicated cases when these macros are still needed, LRA runs
> into infinite loop of such move generation if the macros are not used.
>
>   I might be return to a project to remove necessity of such hooks and
> macros for LRA but I am not sure when.  I guess I need to write a small
> guidance too how to port LRA.
>
>   Also I found that in many cases as here, the macros although working for
> reload can confuse LRA (because of different reload pass and LRA
> implementation approaches).  So I guess the patch is ok although I can not
> approve it as I am not an arm port maintainer.

Thanks for the clarification Vladimir, and BTW I just find the same
cycling issue on iWMMXT.

Yvan
Jeff Law - Nov. 27, 2013, 6:20 p.m.
On 11/27/13 10:58, Vladimir Makarov wrote:
>>
>> The secondary reload macros/hooks define cases where additional
>> registers are needed to reload certain forms of rtl.  I doubt the use
>> of LRA completely eliminates the need for secondary reloads.
>    When I designed LRA I wanted to remove as many hooks as possible
> because I thought insn constraints as major info source are enough.
> Unfortunately I did not succeed fully with secondary reload hooks and
> macros.  It is still needed for some complicated cases but general rule
> to port LRA to a target is to try to switch these hooks off.
I should have remembered some of this.  Didn't we discuss this in the 
context of SECONDARY_MEMORY_NEEDED?


>
>    For example, LRA can generate a move of pseudos of classes for which
> hardware has no real insn.  After that looking on the move constraints,
> LRA can generate more move insns and additional pseudos to generate
> moves (loads or stores if additional pseudos got NO_REGS) which
> represent real hardware insns.  In complicated cases when these macros
> are still needed, LRA runs into infinite loop of such move generation if
> the macros are not used.
So it sounds like LRA reduces the need for the SECONDARY_RELOAD_* 
hooks/macros, but doesn't totally eliminate them.

So I find myself wondering if/how we can document this in the manual. 
Vlad, can you take a stab at that.


>
>    I might be return to a project to remove necessity of such hooks and
> macros for LRA but I am not sure when.  I guess I need to write a small
> guidance too how to port LRA.
That would be good as well, but I think most folks doing port work refer 
more to the GCC internals manual, so we certainly need it updated in 
addition to any "how to port to LRA" kind of document.

Jeff
Jeff Law - Nov. 27, 2013, 6:27 p.m.
On 11/27/13 10:49, Yvan Roux wrote:
>> How can that be correct?
>>
>> The secondary reload macros/hooks define cases where additional registers
>> are needed to reload certain forms of rtl.  I doubt the use of LRA
>> completely eliminates the need for secondary reloads.
>
> Vladimir explained me that in that case on arm, secondary reload hook
> confuses LRA, and that returning NO_REGS will let LRA deal with
> constraints, but I may have badly understand what he said.
So I think with the additional information from Vlad, I think we can go 
forward with your patch -- conditionally approved for the trunk.  The 
condition is giving the ARM maintainers 24hrs to object.

I'm concerned that we don't have good documentation on when these macros 
are still needed in an LRA world.  So it's much more difficult for 
anyone to know if a change of this nature is correct or not.  As you've 
likely seen from my other replies in this thread, I've asked Vlad to 
work on the documentation aspects.

Jeff
Vladimir Makarov - Nov. 27, 2013, 6:36 p.m.
On 11/27/2013, 1:20 PM, Jeff Law wrote:
> On 11/27/13 10:58, Vladimir Makarov wrote:
>>>
>>> The secondary reload macros/hooks define cases where additional
>>> registers are needed to reload certain forms of rtl.  I doubt the use
>>> of LRA completely eliminates the need for secondary reloads.
>>    When I designed LRA I wanted to remove as many hooks as possible
>> because I thought insn constraints as major info source are enough.
>> Unfortunately I did not succeed fully with secondary reload hooks and
>> macros.  It is still needed for some complicated cases but general rule
>> to port LRA to a target is to try to switch these hooks off.
> I should have remembered some of this.  Didn't we discuss this in the 
> context of SECONDARY_MEMORY_NEEDED?
>
>
Yes, we discussed it.  It is just particually one case.  The biggest 
complication I found with secondary memory is PPC port.  Besides this 
macro, only this port uses a special hook saying what exact stack slot 
to use for {SD,DD}mode values.
>>
>>    For example, LRA can generate a move of pseudos of classes for which
>> hardware has no real insn.  After that looking on the move constraints,
>> LRA can generate more move insns and additional pseudos to generate
>> moves (loads or stores if additional pseudos got NO_REGS) which
>> represent real hardware insns.  In complicated cases when these macros
>> are still needed, LRA runs into infinite loop of such move generation if
>> the macros are not used.
> So it sounds like LRA reduces the need for the SECONDARY_RELOAD_* 
> hooks/macros, but doesn't totally eliminate them.
>
> So I find myself wondering if/how we can document this in the manual. 
> Vlad, can you take a stab at that.
>
>
I'll work on it.
>>
>>    I might be return to a project to remove necessity of such hooks and
>> macros for LRA but I am not sure when.  I guess I need to write a small
>> guidance too how to port LRA.
> That would be good as well, but I think most folks doing port work 
> refer more to the GCC internals manual, so we certainly need it 
> updated in addition to any "how to port to LRA" kind of document.
Porting LRA is always unpredictable ride.  But I guess I could classify 
some patterns here.  Only it will be not long, it is not clear area even 
for me.
Yvan Roux - Nov. 27, 2013, 8:31 p.m.
On 27 November 2013 19:27, Jeff Law <law@redhat.com> wrote:
> On 11/27/13 10:49, Yvan Roux wrote:
>>>
>>> How can that be correct?
>>>
>>> The secondary reload macros/hooks define cases where additional registers
>>> are needed to reload certain forms of rtl.  I doubt the use of LRA
>>> completely eliminates the need for secondary reloads.
>>
>>
>> Vladimir explained me that in that case on arm, secondary reload hook
>> confuses LRA, and that returning NO_REGS will let LRA deal with
>> constraints, but I may have badly understand what he said.
>
> So I think with the additional information from Vlad, I think we can go
> forward with your patch -- conditionally approved for the trunk.  The
> condition is giving the ARM maintainers 24hrs to object.
>
> I'm concerned that we don't have good documentation on when these macros are
> still needed in an LRA world.  So it's much more difficult for anyone to
> know if a change of this nature is correct or not.  As you've likely seen
> from my other replies in this thread, I've asked Vlad to work on the
> documentation aspects.

Ok great, as I may play again with these macros to fix the iWMMXT issue.

Yvan
Richard Earnshaw - Nov. 28, 2013, 10:22 a.m.
On 27/11/13 18:27, Jeff Law wrote:
> On 11/27/13 10:49, Yvan Roux wrote:
>>> How can that be correct?
>>>
>>> The secondary reload macros/hooks define cases where additional registers
>>> are needed to reload certain forms of rtl.  I doubt the use of LRA
>>> completely eliminates the need for secondary reloads.
>>
>> Vladimir explained me that in that case on arm, secondary reload hook
>> confuses LRA, and that returning NO_REGS will let LRA deal with
>> constraints, but I may have badly understand what he said.
> So I think with the additional information from Vlad, I think we can go 
> forward with your patch -- conditionally approved for the trunk.  The 
> condition is giving the ARM maintainers 24hrs to object.
> 

This is OK, given the previous discussion.

R.

Patch

diff --git a/gcc/config/arm/arm.h b/gcc/config/arm/arm.h
index 1781b75..d054906 100644
--- a/gcc/config/arm/arm.h
+++ b/gcc/config/arm/arm.h
@@ -1266,11 +1266,12 @@  enum reg_class
 
 /* Must leave BASE_REGS reloads alone */
 #define THUMB_SECONDARY_INPUT_RELOAD_CLASS(CLASS, MODE, X)		\
-  ((CLASS) != LO_REGS && (CLASS) != BASE_REGS				\
-   ? ((true_regnum (X) == -1 ? LO_REGS					\
-       : (true_regnum (X) + HARD_REGNO_NREGS (0, MODE) > 8) ? LO_REGS	\
-       : NO_REGS)) 							\
-   : NO_REGS)
+  (lra_in_progress ? NO_REGS						\
+   : ((CLASS) != LO_REGS && (CLASS) != BASE_REGS			\
+      ? ((true_regnum (X) == -1 ? LO_REGS				\
+         : (true_regnum (X) + HARD_REGNO_NREGS (0, MODE) > 8) ? LO_REGS	\
+         : NO_REGS)) 							\
+      : NO_REGS))
 
 #define THUMB_SECONDARY_OUTPUT_RELOAD_CLASS(CLASS, MODE, X)		\
   ((CLASS) != LO_REGS && (CLASS) != BASE_REGS				\