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

[Ramana Radhakrishnan <ramana dot radhakrishnan at foss dot arm dot 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