This is the mail archive of the
mailing list for the GCC project.
Re: RFC: patch to build GCC for arm with LRA
- From: Yvan Roux <yvan dot roux at linaro dot org>
- To: Richard Earnshaw <rearnsha at arm dot com>
- Cc: Vladimir Makarov <vmakarov at redhat dot com>, gcc-patches <gcc-patches at gcc dot gnu dot org>, "rdsandiford at googlemail dot com" <rdsandiford at googlemail dot com>, Marcus Shawcroft <Marcus dot Shawcroft at arm dot com>, Ramana Radhakrishnan <Ramana dot Radhakrishnan at arm dot com>, Matthew Gretton-Dann <matthew dot gretton-dann at linaro dot org>
- Date: Fri, 30 Aug 2013 15:32:09 +0200
- Subject: Re: RFC: patch to build GCC for arm with LRA
- Authentication-results: sourceware.org; auth=none
- References: <CAD57uCfSsANZvkMHmKphMHRnAxJNXGBxGEmSfhHSYwR7Jg9H-Q at mail dot gmail dot com> <52209BB1 dot 5060801 at arm dot com>
Sorry for the previous off-the-list-html-format answer :(
On 30 August 2013 15:18, Richard Earnshaw <email@example.com> wrote:
> On 30/08/13 14:09, Yvan Roux wrote:
>> here is a request for comments on the 2 attached patches which enable
>> the build of GCC on ARM with LRA. The patches introduce a new
>> undocumented option -mlra to use LRA instead of reload, as what was
>> done on previous LRA support, which is here to ease the test and
>> comparison with reload and not supposed to be kept in 4.9.
>> On AArch32:
>> * The patch includes the previous changes of this thread, add the
>> handling of ASHIFTRT, LSHIFTRT, ROTATE and ROTATERT in get_index_scale
>> and let LRA handle the constraint in Thumb.
>> * The status is that the build complete in ARM mode with a couple of
>> regressions in the testsuite, that I'll investigate:
>> - gcc.c-torture/execute/20060102-1.c
>> - gcc.c-torture/execute/961026-1.c
>> - gcc.target/arm/atomic-comp-swap-release-acquire.c
>> and some build failures in libstdc++ at the pass manager level (there
>> is an invalid read in gt_ggc_mx)
>> * The build of libraries in Thumb mode still doesn't complete, as
>> Vladimir said the secondary_reload modification solves LRA cycling
>> but we still have some issues.
>> On AArch64 :
>> * I modified must_be_index_p to avoid the call of set_address_base
>> with patterns which contains a MULT.
>> * The build complete, but there is a couple of regression in the
>> testsuite I'm looking at on
>> - gcc.c-torture/execute/ieee/fp-cmp-4l.c
>> - c-c++-common/torture/complex-sign-mul-minus-one.c
>> for instance.
>> Any comments ?
> Why are you posting to conflicting sets of patches for rtlanal.c?
> Changes to that file will have to work for all architectures; so while
> it helps a little bit to see which changes are needed for which target,
> ultimately you need one patch for that file that works everywhere.
Yes sorry for that, I've 2 dev branches on my side for AArch32 and
AArch64, and I thought that posting one patch for each will help
people who wants to test one target in particular, but I planned to
make either one patch for switching ARM on LRA at the end or 2 patches
> Also, as a style nit, use
> return (test
> || test
> || test);
> so that the parenthesis will force correct indentation of continuation
Ok, will fix this.
>> On 6 July 2013 01:12, Vladimir Makarov <firstname.lastname@example.org> wrote:
>>> On 13-07-05 8:43 AM, Yvan Roux wrote:
>>>> 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,
>>> 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.
>>> N ÂnârÂÂÃÃ)emÃhÃyhiÃ Âw^âÂÃ
>>> N ÂnârÂÂÃÃ)emÃhÃyhiÃ Âw^âÂÃ