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

Kyrill Tkachov kyrylo.tkachov@foss.arm.com
Fri Jan 6 16:46:00 GMT 2017


On 05/01/17 12:09, Kyrill Tkachov wrote:
>
> 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.
>

Here it is. Bootstrapped and tested on arm-none-linux-gnueabihf and aarch64-none-linux-gnu.
Ok?

Thanks,
Kyrill

2017-01-06  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * tree-ssa-address.c (gen_addr_rtx): Don't handle index if it
     is const0_rtx.

2017-01-06  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.dg/20161219.c: New test.

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: addr.patch
Type: text/x-patch
Size: 1043 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20170106/7dad9837/attachment.bin>


More information about the Gcc-patches mailing list