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

[Ramana Radhakrishnan <ramana.radhakrishnan@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.
>


We shouldn't generate any ldm / stm operations for anything other than 
memcpy before reload and all of those are capped at 4 integer registers 
(MAX_LDM_STM_OPS in arm.h) at a time and that will be handled almost 
entirely by the patterns in ldmstm.md. The way to generate this 
generically this would be through movmem expanders or through the 
load_multiple expander in the backend. However all of this falls through 
the arm_gen_load_multiple interface all of which goes through 
arm_gen_multiple_op and we have an assert in arm_gen_multiple_op that 
the number of registers is <= MAX_LDM_STM_OPS.

Thus OK, please apply but be aware of any fallout

Thanks,
Ramana


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