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

[Richard Biener <richard dot guenther at gmail dot com>]

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)

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.
>>
>>
>