Sorry for the previous off-the-list-html-format answer :(
On 30 August 2013 15:18, Richard Earnshaw <rearn...@arm.com> wrote:
> On 30/08/13 14:09, Yvan Roux wrote:
>> Hi,
>>
>> 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
without conflict.
> Also, as a style nit, use
>
> return (test
> || test
> || test);
>
> so that the parenthesis will force correct indentation of continuation
> lines.
Ok, will fix this.
Thanks Richard
>
> R.
>
>> Thanks
>> Yvan
>>
>>
>>
>>
>> On 6 July 2013 01:12, Vladimir Makarov <vmaka...@redhat.com> wrote:
>>> 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.
>>> =
>>>
>>> aarch32-lra.patch
>>>
>>>
>>> N ¬n‡r¥ªíÂ)emçhÂyhi× ¢w^™©Ý
>>>
>>> aarch64-lra.patch
>>>
>>>
>>> N ¬n‡r¥ªíÂ)emçhÂyhi× ¢w^™©Ý
>
>
