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

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

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)

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

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