Re: [PATCH][tree-ssa-address] Use simplify_gen_binary in gen_addr_rtx

[Kyrill Tkachov <kyrylo dot tkachov at foss dot arm dot com>]

On 05/01/17 12:01, Richard Biener wrote:
> On Wed, Jan 4, 2017 at 4:07 PM, Kyrill Tkachov
> <kyrylo.tkachov@foss.arm.com> wrote:
> > On 04/01/17 14:19, Richard Biener wrote:
> > > On Wed, Dec 21, 2016 at 10:40 AM, Kyrill Tkachov
> > > <kyrylo.tkachov@foss.arm.com> wrote:
> > > > On 20/12/16 17:30, Richard Biener wrote:
> > > > > On December 20, 2016 5:01:19 PM GMT+01:00, Kyrill Tkachov
> > > > > <kyrylo.tkachov@foss.arm.com> wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > The testcase in this patch generates bogus assembly for arm with -O1
> > > > > > -mfloat-abi=soft:
> > > > > >           strd    r4, [#0, r3]
> > > > > >
> > > > > > This is due to non-canonical RTL being generated during expansion:
> > > > > > (set (mem:DI (plus:SI (const_int 0 [0])
> > > > > >      (reg/f:SI 153)) [0 MEM[symbol: a, index: _26, offset: 0B]+0 S8
> > > > > > A64])
> > > > > >            (reg:DI 154))
> > > > > >
> > > > > > Note the (plus (const_int 0) (reg)). This is being generated in
> > > > > > gen_addr_rtx in tree-ssa-address.c
> > > > > > where it creates an explicit PLUS rtx through gen_rtx_PLUS, which
> > > > > > doesn't try to canonicalise its arguments
> > > > > > or simplify. The correct thing to do is to use simplify_gen_binary that
> > > > > > will handle all this properly.
> > > > > But it has to match up the validity check which passes down exactly the
> > > > > same RTL(?)  Or does this stem from propagation simplifying a MEM after
> > > > > IVOPTs?
> > > >
> > > > You mean TARGET_LEGITIMATE_ADDRESS_P? Yes, it gets passed on to that, but
> > > > the arm implementation of that
> > > > doesn't try to handle non-canonical RTL (plus (const0_rtx) (reg) is not
> > > > canonical).
> > > > Or do you mean some other check?
> > > Ok, now looking at the actual patch and the code in question.  For your
> > > testcase
> > > it happens that symbol == const0_rtx?  In this case please amend the
> > > if (symbol) check like we do for the base, thus
> > >
> > >      if (symbol && symbol != const0_rtx)
> >
> > No, symbol is not const0_rtx (it's just a symbol_ref).
> > index is const0_rtx and so gets assigned to *addr at the beginning of the
> > function.
> > base and step are NULL_RTX.
> > So at the time of the check:
> >         if (*addr)
> >           *addr = gen_rtx_PLUS (address_mode, *addr, act_elem);
> >         else
> >           *addr = act_elem;
> >
> > *addr is const0_rtx. Do you want to amend that check to:
> >      if (*addr && *addr != const0_rtx) ?
> Hmm, I guess given the if (step) in if (index) *addr can end up being
> a not simplified mult.  So instead do
>
>     if (index && index != const0_rtx)

That works, I'll test a patch for this.

> > I haven't looked where the const0_rtx index comes from, so I don't know if
> > it
> > could have other CONST_INT values that may cause trouble.
> Usually this happens when constant folding / propagation happens after
> IVOPTs generates the TARGET_MEM_REF.  We do have some canonicalization
> foldings for TMR, see maybe_fold_tmr, but that should have made index NULL
> if it was constant...  So maybe we fail to fold a stmt at some point.
>
> Btw, I fail to see the bogus asm with my arm-non-eabi cross with -O
> -mfloat-abi=soft
> so I can't tell how the TMR arrives at RTL expansion.

You'll also want to specify -marm (this doesn't trigger on Thumb) and perhaps -march=armv7-a.

Thanks,
Kyrill

> Richard.
>
> > Kyrill
> >
> >
> > > Richard.
> > >
> > > > Thanks,
> > > > Kyrill
> > > >
> > > >
> > > > > > I didn't change the other gen_rtx_PLUS calls in this function as their
> > > > > > results is later used in XEXP operations
> > > > > > that seem to rely on a PLUS expression being explicitly produced, but
> > > > > > this particular call doesn't, so it's okay
> > > > > > to change it. With this patch the sane assembly is generated:
> > > > > >            strd    r4, [r3]
> > > > > >
> > > > > > Bootstrapped and tested on arm-none-linux-gnueabihf, x86_64,
> > > > > > aarch64-none-linux-gnu.
> > > > > >
> > > > > > Ok for trunk?
> > > > > >
> > > > > > Thanks,
> > > > > > Kyrill
> > > > > >
> > > > > > 2016-12-20  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> > > > > >
> > > > > >       * tree-ssa-address.c (gen_addr_rtx): Use simplify_gen_binary to
> > > > > > add
> > > > > >        *addr to act_elem.
> > > > > >
> > > > > > 2016-12-20  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
> > > > > >
> > > > > >        * gcc.dg/20161219.c: New test.