Re: RFA: patch to build GCC for arm with LRA
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.
Re: RFA: patch to build GCC for arm with LRA
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 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 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 > > * 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. aarch64-lra.diff Description: Binary data
Re: RFA: patch to build GCC for arm with LRA
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 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 * 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.
Re: RFA: patch to build GCC for arm with LRA
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 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 * 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.
Re: RFA: patch to build GCC for arm with LRA
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 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 >> >> * 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.
Re: RFA: patch to build GCC for arm with LRA
On 27/06/13 17:32, Vladimir Makarov wrote:
On 06/27/2013 12:15 PM, Richard Sandiford wrote:
Vladimir Makarov 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
* 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)
RFA: patch to build GCC for arm with LRA
On 06/27/2013 12:15 PM, Richard Sandiford wrote:
> Vladimir Makarov 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
* rtlanal.c (must_be_index_p, set_address_index): Add ASHIFTRT,
LSHIFTRT, and ROTATERT.
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)
