Patchwork [4.9,alpha] : Switch alpha to LRA

login
register
mail settings
Submitter Chung-Ju Wu
Date April 25, 2013, 8:04 a.m.
Message ID <CADj25HNmQLe5HTW8Loa7vKJQpww5xypeZgr-YidRik29F0xZ=g@mail.gmail.com>
Download mbox | patch
Permalink /patch/239424/
State New
Headers show

Comments

Chung-Ju Wu - April 25, 2013, 8:04 a.m.
2013/4/23 Steven Bosscher <stevenb.gcc@gmail.com>:
> On Mon, Apr 22, 2013 at 7:33 PM, Jeff Law wrote:
>> On 04/22/2013 11:17 AM, Uros Bizjak wrote:
>>>
>>> On Tue, Jan 29, 2013 at 12:34 AM, Richard Henderson <rth@redhat.com>
>>> wrote:
>>>>
>>>> On 01/28/2013 03:14 PM, Uros Bizjak wrote:
>>>>>
>>>>>
>>>>> 2013-01-28  Uros Bizjak<ubizjak@gmail.com>
>>>>>
>>>>>          * config/alpha/alpha.c (TARGET_LRA_P): New define.
>>>>>
>>>>> Bootstrapped and regression tested [1] on alphaev68-unknown-linux-gnu.
>>>>>
>>>>> OK for 4.9?
>>>>>
>>>>
>>>> Yep.
>>>
>>>
>>> Unfortunately, alphas are much more tied to reload than it was hoped.
>>> While latest alphas (with FIX and BWX ISAs) survived transition to LRA
>>> without problems, further testing on ev4 and ev5 triggered various
>>> problems, one of them is PR57032 [1] that exposed rather unique way of
>>> handling aligned/nonaligned memory operands.
>>>
>>> The patch was reverted.
>>>
>>> I suspect that fixing older alphas to live with LRA would be quite
>>> involved task, and I guess nobody (including me) wants to spend
>>> considerable amount of time on a dying architecture. Consequently,
>>> this also means that alphas will die together with reload as far as
>>> gcc is concerned.
>>>
>>> [1] http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57032
>>
>> Would it make sense to deprecate the older Alpha implementations without
>> killing the "modern" ones?
>
> Right. That would also eliminate the NOTE_INSN_EH_REGION notes bug (PR
> target/56858).
>
> I think it would be a shame to not enable LRA on alpha. It will only
> be another excuse to never let reload die, and it will hurt stability
> and reliability for Alpha EV6 in the long term as other targets switch
> over to LRA.
>
> Is it possible to add some EV4/EV5 splitters to work around this Alpha
> EV4/EV5 oddity? Even if it comes at a code quality cost, it's IMHO
> still better than tying the fate of apha to reload and vice versa..
>
> Ciao!
> Steven


How about using follow constraints?



As Uros said, the 'Q' is ignored by LRA.
The reason is that the predicate function normal_memory_operand()
may change op to a memory location by using resolved_reload_operand().
However, if LRA is enabled, resolve_reload_operand() always returns
original reg op because reload_in_progress would never be true,
resulting reload loop in this case.

So I guess using 'm' constraint instead of 'Q' is able to avoid
such abnormal behavior, leaving all the reload jobs to LRA.
IMHO that is a simplest solution.  At least it passes the case in
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57032
and successfully build libgcc.


Best regards,
jasonwucj
Uros Bizjak - April 25, 2013, 8:18 a.m.
On Thu, Apr 25, 2013 at 10:04 AM, Chung-Ju Wu <jasonwucj@gmail.com> wrote:

>>>>>>          * config/alpha/alpha.c (TARGET_LRA_P): New define.
>>>>>>
>>>>>> Bootstrapped and regression tested [1] on alphaev68-unknown-linux-gnu.
>>>>>>
>>>>>> OK for 4.9?
>>>>>>
>>>>>
>>>>> Yep.
>>>>
>>>>
>>>> Unfortunately, alphas are much more tied to reload than it was hoped.
>>>> While latest alphas (with FIX and BWX ISAs) survived transition to LRA
>>>> without problems, further testing on ev4 and ev5 triggered various
>>>> problems, one of them is PR57032 [1] that exposed rather unique way of
>>>> handling aligned/nonaligned memory operands.
>>>>
>>>> The patch was reverted.
>>>>
>>>> I suspect that fixing older alphas to live with LRA would be quite
>>>> involved task, and I guess nobody (including me) wants to spend
>>>> considerable amount of time on a dying architecture. Consequently,
>>>> this also means that alphas will die together with reload as far as
>>>> gcc is concerned.
>>>>
>>>> [1] http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57032
>>>
>>> Would it make sense to deprecate the older Alpha implementations without
>>> killing the "modern" ones?
>>
>> Right. That would also eliminate the NOTE_INSN_EH_REGION notes bug (PR
>> target/56858).
>>
>> I think it would be a shame to not enable LRA on alpha. It will only
>> be another excuse to never let reload die, and it will hurt stability
>> and reliability for Alpha EV6 in the long term as other targets switch
>> over to LRA.
>>
>> Is it possible to add some EV4/EV5 splitters to work around this Alpha
>> EV4/EV5 oddity? Even if it comes at a code quality cost, it's IMHO
>> still better than tying the fate of apha to reload and vice versa..
>>
>> Ciao!
>> Steven
>
>
> How about using follow constraints?
>
> Index: alpha.c
> ===================================================================
> --- alpha.c     (revision 198216)
> +++ alpha.c     (working copy)
> @@ -9871,6 +9871,9 @@
>  #undef TARGET_LEGITIMATE_ADDRESS_P
>  #define TARGET_LEGITIMATE_ADDRESS_P alpha_legitimate_address_p
>
> +#undef TARGET_LRA_P
> +#define TARGET_LRA_P hook_bool_void_true
> +
>  #undef TARGET_CONDITIONAL_REGISTER_USAGE
>  #define TARGET_CONDITIONAL_REGISTER_USAGE alpha_conditional_register_usage
>
> Index: alpha.md
> ===================================================================
> --- alpha.md    (revision 198216)
> +++ alpha.md    (working copy)
> @@ -4073,9 +4073,9 @@
>
>  (define_insn "*movdi"
>    [(set (match_operand:DI 0 "nonimmediate_operand"
> -                               "=r,r,r,r,r,r,r,r, m, *f,*f, Q, r,*f")
> +                               "=r,r,r,r,r,r,r,r, m, *f,*f, m, r,*f")
>         (match_operand:DI 1 "input_operand"
> -                               "rJ,K,L,T,s,n,s,m,rJ,*fJ, Q,*f,*f, r"))]
> +                               "rJ,K,L,T,s,n,s,m,rJ,*fJ, m,*f,*f, r"))]
>    "register_operand (operands[0], DImode)
>     || reg_or_0_operand (operands[1], DImode)"
>    "@
>
>
> As Uros said, the 'Q' is ignored by LRA.
> The reason is that the predicate function normal_memory_operand()
> may change op to a memory location by using resolved_reload_operand().
> However, if LRA is enabled, resolve_reload_operand() always returns
> original reg op because reload_in_progress would never be true,
> resulting reload loop in this case.
>
> So I guess using 'm' constraint instead of 'Q' is able to avoid
> such abnormal behavior, leaving all the reload jobs to LRA.
> IMHO that is a simplest solution.  At least it passes the case in
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=57032
> and successfully build libgcc.

Yes, this patch will work up to building libstdc++, where it will fail
with the same reason on movqi pattern (on non-BWX target). I suspect
that QImode access is generated during LRA (where reload_in_progress
is false!) and it directly generates movqi, which can't use memory
operand. Maybe simply adding lra_in_progress to reload_in_progress
would fix this problem.

Uros.
Richard Henderson - April 25, 2013, 12:31 p.m.
On 2013-04-25 09:18, Uros Bizjak wrote:
> Yes, this patch will work up to building libstdc++, where it will fail
> with the same reason on movqi pattern (on non-BWX target). I suspect
> that QImode access is generated during LRA (where reload_in_progress
> is false!) and it directly generates movqi, which can't use memory
> operand. Maybe simply adding lra_in_progress to reload_in_progress
> would fix this problem.

Possibly.

I've wondered in the past if it wouldn't be better to spill
QI/HImode values in SImode for pre-BWX targets.  Back in the
day that would have involved hacking reload, but now it would
appear that SECONDARY_MEMORY_NEEDED_RTX plus a reload_[qh]i
pattern would do the job.

Of course, that macro itself is in reload.c... Is there some
other way we're supposed to get the same effect from LRA?


r~

Patch

Index: alpha.c
===================================================================
--- alpha.c     (revision 198216)
+++ alpha.c     (working copy)
@@ -9871,6 +9871,9 @@ 
 #undef TARGET_LEGITIMATE_ADDRESS_P
 #define TARGET_LEGITIMATE_ADDRESS_P alpha_legitimate_address_p

+#undef TARGET_LRA_P
+#define TARGET_LRA_P hook_bool_void_true
+
 #undef TARGET_CONDITIONAL_REGISTER_USAGE
 #define TARGET_CONDITIONAL_REGISTER_USAGE alpha_conditional_register_usage

Index: alpha.md
===================================================================
--- alpha.md    (revision 198216)
+++ alpha.md    (working copy)
@@ -4073,9 +4073,9 @@ 

 (define_insn "*movdi"
   [(set (match_operand:DI 0 "nonimmediate_operand"
-                               "=r,r,r,r,r,r,r,r, m, *f,*f, Q, r,*f")
+                               "=r,r,r,r,r,r,r,r, m, *f,*f, m, r,*f")
        (match_operand:DI 1 "input_operand"
-                               "rJ,K,L,T,s,n,s,m,rJ,*fJ, Q,*f,*f, r"))]
+                               "rJ,K,L,T,s,n,s,m,rJ,*fJ, m,*f,*f, r"))]
   "register_operand (operands[0], DImode)
    || reg_or_0_operand (operands[1], DImode)"
   "@