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.
Also, as a style nit, use
return (test
|| test
|| test);
so that the parenthesis will force correct indentation of continuation
lines.
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^™©Ý
