Patchwork RFA: patch to build GCC for arm with LRA

login
register
mail settings
Submitter Vladimir Makarov
Date June 27, 2013, 4:32 p.m.
Message ID <51CC6908.1040005@redhat.com>
Download mbox | patch
Permalink /patch/255109/
State New
Headers show

Comments

Vladimir Makarov - June 27, 2013, 4:32 p.m.
On 06/27/2013 12:15 PM, Richard Sandiford wrote:
> Vladimir Makarov <vmakarov@redhat.com> writes:
>> Richard, is it ok to commit to the trunk?
> Looks good to me, but I gave up the right to approve it.  I think you
> need ROTATERT too though (see arm_legitimate_index_p).
>
> Also, sorry for the nitpick, but once the full condition overflows one line,
> I think each == test should be on its own line.
>
>
Thanks for the comments.  Here is the new version of the patch:

2013-06-27  Vladimir Makarov  <vmakarov@redhat.com>

        * rtlanal.c (must_be_index_p, set_address_index): Add ASHIFTRT,
        LSHIFTRT, and ROTATERT.
Richard Earnshaw - June 27, 2013, 4:50 p.m.
On 27/06/13 17:32, Vladimir Makarov wrote:
> On 06/27/2013 12:15 PM, Richard Sandiford wrote:
>> Vladimir Makarov <vmakarov@redhat.com> writes:
>>> Richard, is it ok to commit to the trunk?
>> Looks good to me, but I gave up the right to approve it.  I think you
>> need ROTATERT too though (see arm_legitimate_index_p).
>>
>> Also, sorry for the nitpick, but once the full condition overflows one line,
>> I think each == test should be on its own line.
>>
>>
> Thanks for the comments.  Here is the new version of the patch:
>
> 2013-06-27  Vladimir Makarov  <vmakarov@redhat.com>
>
>          * rtlanal.c (must_be_index_p, set_address_index): Add ASHIFTRT,
>          LSHIFTRT, and ROTATERT.
>

Although it's not needed for ARM, why would you leave out ROTATE?

Hmm, on second thoughts ROTATERT immediate is always canonicalized to 
ROTATE (Pmode-size - imm), so it might be needed on ARM too.

R.

>
> arm2.patch
>
>
> Index: rtlanal.c
> ===================================================================
> --- rtlanal.c	(revision 200174)
> +++ rtlanal.c	(working copy)
> @@ -5480,7 +5480,11 @@ must_be_base_p (rtx x)
>   static bool
>   must_be_index_p (rtx x)
>   {
> -  return GET_CODE (x) == MULT || GET_CODE (x) == ASHIFT;
> +  return (GET_CODE (x) == MULT
> +	  || GET_CODE (x) == ASHIFT
> +	  || GET_CODE (x) == ASHIFTRT
> +	  || GET_CODE (x) == LSHIFTRT
> +	  || GET_CODE (x) == ROTATERT);
>   }
>
>   /* Set the segment part of address INFO to LOC, given that INNER is the
> @@ -5519,7 +5523,11 @@ set_address_base (struct address_info *i
>   static void
>   set_address_index (struct address_info *info, rtx *loc, rtx *inner)
>   {
> -  if ((GET_CODE (*inner) == MULT || GET_CODE (*inner) == ASHIFT)
> +  if ((GET_CODE (*inner) == MULT
> +       || GET_CODE (*inner) == ASHIFT
> +       || GET_CODE (*inner) == ASHIFTRT
> +       || GET_CODE (*inner) == LSHIFTRT
> +       || GET_CODE (*inner) == ROTATERT)
>         && CONSTANT_P (XEXP (*inner, 1)))
>       inner = strip_address_mutations (&XEXP (*inner, 0));
>     gcc_checking_assert (REG_P (*inner)
>
Vladimir Makarov - June 27, 2013, 4:59 p.m.
On 06/27/2013 12:50 PM, Richard Earnshaw wrote:
> On 27/06/13 17:32, Vladimir Makarov wrote:
>> On 06/27/2013 12:15 PM, Richard Sandiford wrote:
>>> Vladimir Makarov <vmakarov@redhat.com> writes:
>>>> Richard, is it ok to commit to the trunk?
>>> Looks good to me, but I gave up the right to approve it.  I think you
>>> need ROTATERT too though (see arm_legitimate_index_p).
>>>
>>> Also, sorry for the nitpick, but once the full condition overflows
>>> one line,
>>> I think each == test should be on its own line.
>>>
>>>
>> Thanks for the comments.  Here is the new version of the patch:
>>
>> 2013-06-27  Vladimir Makarov  <vmakarov@redhat.com>
>>
>>          * rtlanal.c (must_be_index_p, set_address_index): Add ASHIFTRT,
>>          LSHIFTRT, and ROTATERT.
>>
>
> Although it's not needed for ARM, why would you leave out ROTATE?
>
> Hmm, on second thoughts ROTATERT immediate is always canonicalized to
> ROTATE (Pmode-size - imm), so it might be needed on ARM too.
Thanks, Richard.  I guess we can include ROTATE.  It definitely will not
hurt but it might be useful for other targets too.

So I added ROTATE to the patch and like to get approval for it too.
Richard Earnshaw - June 27, 2013, 5:10 p.m.
On 27/06/13 17:59, Vladimir Makarov wrote:
> On 06/27/2013 12:50 PM, Richard Earnshaw wrote:
>> On 27/06/13 17:32, Vladimir Makarov wrote:
>>> On 06/27/2013 12:15 PM, Richard Sandiford wrote:
>>>> Vladimir Makarov <vmakarov@redhat.com> writes:
>>>>> Richard, is it ok to commit to the trunk?
>>>> Looks good to me, but I gave up the right to approve it.  I think you
>>>> need ROTATERT too though (see arm_legitimate_index_p).
>>>>
>>>> Also, sorry for the nitpick, but once the full condition overflows
>>>> one line,
>>>> I think each == test should be on its own line.
>>>>
>>>>
>>> Thanks for the comments.  Here is the new version of the patch:
>>>
>>> 2013-06-27  Vladimir Makarov  <vmakarov@redhat.com>
>>>
>>>           * rtlanal.c (must_be_index_p, set_address_index): Add ASHIFTRT,
>>>           LSHIFTRT, and ROTATERT.
>>>
>>
>> Although it's not needed for ARM, why would you leave out ROTATE?
>>
>> Hmm, on second thoughts ROTATERT immediate is always canonicalized to
>> ROTATE (Pmode-size - imm), so it might be needed on ARM too.
> Thanks, Richard.  I guess we can include ROTATE.  It definitely will not
> hurt but it might be useful for other targets too.
>
> So I added ROTATE to the patch and like to get approval for it too.
>
>

Oh, and another thought, AArch64 will probably need ZERO_EXTEND and 
SIGN_EXTEND as well.

R.
Vladimir Makarov - June 27, 2013, 5:21 p.m.
On 06/27/2013 01:10 PM, Richard Earnshaw wrote:
> On 27/06/13 17:59, Vladimir Makarov wrote:
>> On 06/27/2013 12:50 PM, Richard Earnshaw wrote:
>>> On 27/06/13 17:32, Vladimir Makarov wrote:
>>>> On 06/27/2013 12:15 PM, Richard Sandiford wrote:
>>>>> Vladimir Makarov <vmakarov@redhat.com> writes:
>>>>>> Richard, is it ok to commit to the trunk?
>>>>> Looks good to me, but I gave up the right to approve it.  I think you
>>>>> need ROTATERT too though (see arm_legitimate_index_p).
>>>>>
>>>>> Also, sorry for the nitpick, but once the full condition overflows
>>>>> one line,
>>>>> I think each == test should be on its own line.
>>>>>
>>>>>
>>>> Thanks for the comments.  Here is the new version of the patch:
>>>>
>>>> 2013-06-27  Vladimir Makarov  <vmakarov@redhat.com>
>>>>
>>>>           * rtlanal.c (must_be_index_p, set_address_index): Add
>>>> ASHIFTRT,
>>>>           LSHIFTRT, and ROTATERT.
>>>>
>>>
>>> Although it's not needed for ARM, why would you leave out ROTATE?
>>>
>>> Hmm, on second thoughts ROTATERT immediate is always canonicalized to
>>> ROTATE (Pmode-size - imm), so it might be needed on ARM too.
>> Thanks, Richard.  I guess we can include ROTATE.  It definitely will not
>> hurt but it might be useful for other targets too.
>>
>> So I added ROTATE to the patch and like to get approval for it too.
>>
>>
>
> Oh, and another thought, AArch64 will probably need ZERO_EXTEND and
> SIGN_EXTEND as well.
>
It is already implemented as many targets use it.
Yvan Roux - July 5, 2013, 12:43 p.m.
Hi,

for AArch64 it is also needed to take into account SIGN_EXTRACT in the
set_address_base and set_address_index routines, as we acan encounter
that kind of insn for instance :

(insn 29 27 5 7 (set (mem:SI (plus:DI (sign_extract:DI (mult:DI
(subreg:DI (reg/v:SI 76 [ elt ]) 0)
...

with the attached patch and the LRA enabled, compiler now bootstrap
but I've few regressions in the testsuite,
gcc.c/torture/execute/fp-cmp-4l.c for instance. I was looking at these
issues before submitting  a complete AArch64 LRA enabling patch, but
as you are speaking about that...

Valdimir, for the ARM target I already had the ASHIFTRT and LSHIFTRT
addition on my side, but still had an ICE during bootstrap with LRA
when compiling fixed-bit.c (the Max number of generated reload insns
we talk about already) is it working for you ?

Thanks,
Yvan

On 27 June 2013 19:21, Vladimir Makarov <vmakarov@redhat.com> wrote:
> On 06/27/2013 01:10 PM, Richard Earnshaw wrote:
>> On 27/06/13 17:59, Vladimir Makarov wrote:
>>> On 06/27/2013 12:50 PM, Richard Earnshaw wrote:
>>>> On 27/06/13 17:32, Vladimir Makarov wrote:
>>>>> On 06/27/2013 12:15 PM, Richard Sandiford wrote:
>>>>>> Vladimir Makarov <vmakarov@redhat.com> writes:
>>>>>>> Richard, is it ok to commit to the trunk?
>>>>>> Looks good to me, but I gave up the right to approve it.  I think you
>>>>>> need ROTATERT too though (see arm_legitimate_index_p).
>>>>>>
>>>>>> Also, sorry for the nitpick, but once the full condition overflows
>>>>>> one line,
>>>>>> I think each == test should be on its own line.
>>>>>>
>>>>>>
>>>>> Thanks for the comments.  Here is the new version of the patch:
>>>>>
>>>>> 2013-06-27  Vladimir Makarov  <vmakarov@redhat.com>
>>>>>
>>>>>           * rtlanal.c (must_be_index_p, set_address_index): Add
>>>>> ASHIFTRT,
>>>>>           LSHIFTRT, and ROTATERT.
>>>>>
>>>>
>>>> Although it's not needed for ARM, why would you leave out ROTATE?
>>>>
>>>> Hmm, on second thoughts ROTATERT immediate is always canonicalized to
>>>> ROTATE (Pmode-size - imm), so it might be needed on ARM too.
>>> Thanks, Richard.  I guess we can include ROTATE.  It definitely will not
>>> hurt but it might be useful for other targets too.
>>>
>>> So I added ROTATE to the patch and like to get approval for it too.
>>>
>>>
>>
>> Oh, and another thought, AArch64 will probably need ZERO_EXTEND and
>> SIGN_EXTEND as well.
>>
> It is already implemented as many targets use it.
Vladimir Makarov - July 5, 2013, 11:12 p.m.
On 13-07-05 8:43 AM, Yvan Roux wrote:
> Hi,
>
> for AArch64 it is also needed to take into account SIGN_EXTRACT in the
> set_address_base and set_address_index routines, as we acan encounter
> that kind of insn for instance :
>
> (insn 29 27 5 7 (set (mem:SI (plus:DI (sign_extract:DI (mult:DI
> (subreg:DI (reg/v:SI 76 [ elt ]) 0)
> ...
OK.
> with the attached patch and the LRA enabled, compiler now bootstrap
> but I've few regressions in the testsuite,
It seems ok to me but I am confused of the following change:

  set_address_base (struct address_info *info, rtx *loc, rtx *inner)
  {
-  if (GET_CODE (*inner) == LO_SUM)
+  if (GET_CODE (*inner) == SIGN_EXTRACT)
+    inner = strip_address_mutations (&XEXP (*inner, 0));
+
+  if (GET_CODE (*inner) == LO_SUM || GET_CODE (*inner) == MULT)
      inner = strip_address_mutations (&XEXP (*inner, 0));
    gcc_checking_assert (REG_P (*inner)
                 || MEM_P (*inner)

base address should not contain MULT (which you added).  It is 
controlled by the result of must_be_index_p.  So set_address_base should 
not have code for MULT and you need to change must_be_index_p in a way 
that set_address_base is not called for MULT.

> gcc.c/torture/execute/fp-cmp-4l.c for instance. I was looking at these
> issues before submitting  a complete AArch64 LRA enabling patch, but
> as you are speaking about that...
>
> Valdimir, for the ARM target I already had the ASHIFTRT and LSHIFTRT
> addition on my side, but still had an ICE during bootstrap with LRA
> when compiling fixed-bit.c (the Max number of generated reload insns
> we talk about already) is it working for you ?
>
>
I did not check thumb I guess.  If what you are asking about the problem 
you sent me about 2 weeks ago, I guess one solution would be putting

   if (lra_in_progress)
     return NO_REGS;

at the beginning of arm/targhooks.c::default_secondary_reload.  LRA is 
smart enough to figure out what to do from constraints in most cases of 
when reload needs secondary_reload.  In arm case, 
default_secondary_reload only confuses LRA.

I had no time to test this change, but it solves LRA cycling for the 
test case you sent me.

Patch

Index: rtlanal.c
===================================================================
--- rtlanal.c	(revision 200174)
+++ rtlanal.c	(working copy)
@@ -5480,7 +5480,11 @@  must_be_base_p (rtx x)
 static bool
 must_be_index_p (rtx x)
 {
-  return GET_CODE (x) == MULT || GET_CODE (x) == ASHIFT;
+  return (GET_CODE (x) == MULT
+	  || GET_CODE (x) == ASHIFT
+	  || GET_CODE (x) == ASHIFTRT
+	  || GET_CODE (x) == LSHIFTRT
+	  || GET_CODE (x) == ROTATERT);
 }
 
 /* Set the segment part of address INFO to LOC, given that INNER is the
@@ -5519,7 +5523,11 @@  set_address_base (struct address_info *i
 static void
 set_address_index (struct address_info *info, rtx *loc, rtx *inner)
 {
-  if ((GET_CODE (*inner) == MULT || GET_CODE (*inner) == ASHIFT)
+  if ((GET_CODE (*inner) == MULT
+       || GET_CODE (*inner) == ASHIFT
+       || GET_CODE (*inner) == ASHIFTRT
+       || GET_CODE (*inner) == LSHIFTRT
+       || GET_CODE (*inner) == ROTATERT)
       && CONSTANT_P (XEXP (*inner, 1)))
     inner = strip_address_mutations (&XEXP (*inner, 0));
   gcc_checking_assert (REG_P (*inner)