Re: [PATCH][ARM] PR target/71436: Restrict *load_multiple pattern till after LRA

[Kyrill Tkachov <kyrylo.tkachov@foss.arm.com>]

On 07/02/17 14:49, Kyrill Tkachov wrote:
>
> On 18/01/17 09:49, Kyrill Tkachov wrote:
>>
>> On 19/12/16 14:53, Jakub Jelinek wrote:
>>> On Thu, Dec 15, 2016 at 10:00:14AM +0000, Richard Earnshaw (lists) wrote:
>>>> sorry, pasted the wrong bit of code.
>>>>
>>>> That should read when we generate:
>>>>
>>>> (insn 55 19 67 3 (parallel [
>>>>              (set (reg:SI 0 r0)
>>>>                  (mem/u/c:SI (reg/f:SI 147) [2 c+0 S4 A32]))
>>>>              (set (reg:SI 158)
>>>>                  (mem/u/c:SI (plus:SI (reg/f:SI 147)
>>>>                          (const_int 4 [0x4])) [2 c+4 S4 A32]))
>>>>          ]) "ldm.c":25 404 {*load_multiple}
>>>>       (expr_list:REG_UNUSED (reg:SI 0 r0)
>>>>          (nil)))
>>>>
>>>> ie when we put a pseudo into the register load list.
>>> We put a pseudo there because the predicate on the insn allows it:
>>> (define_special_predicate "load_multiple_operation"
>>>    (match_code "parallel")
>>> {
>>>   return ldm_stm_operation_p (op, /*load=*/true, SImode,
>>>                                   /*consecutive=*/false,
>>>                                   /*return_pc=*/false);
>>> })
>>> and the consecutive = false argument says that (almost) no verification
>>> is performed on the SET_DEST, just that it is a REG and doesn't have
>>> REGNO smaller than the first reg.
>>> That said, RA is still not able to cope with such instructions, because
>>> only the first set is represented with constraints, so if such an insn
>>> needs any kind of reloading, it just will not happen.
>>> So I think the posted patch makes lots of sense, otherwise if you use
>>> such a pattern before reload, you just have to hope no reloading will be
>>> needed on it.
>>
>> In that case I believe restricting the pattern till after LRA is the way to go.
>> However, we should still add the check from [1] to ldm_stm_operation_p for when
>> 'consecutive' is true as consecutive order has no useful meaning on pseudos here.
>>
>
> In the interest of fixing this PR I think the patch at https://gcc.gnu.org/ml/gcc-patches/2016-11/msg03078.html
> is the way to go, that is disable this pattern until after reload.
> It was intended to handle epilogue pops that is generated after reload anyway and all the general load/store multiple
> patterns are handled by ldmstmd.md so not having this before reload is not risky.
>
> I'll also propose a variant of https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01400.html to tighten up the consecutivity
> check when 'consecutive' is passed down as true, but that is indepdendent of this PR.
>
> Is the patch at https://gcc.gnu.org/ml/gcc-patches/2016-11/msg03078.html ok for trunk?
> It's been in my tree for a couple of months now getting testing and there's been no fallout.
>

Ping.
Thanks,
Kyrill

> Thanks,
> Kyrill
>
>>
>>
>> [1] https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01400.html
>>
>>>
>>>     Jakub
>>
>