On Wed, Jan 4, 2017 at 4:07 PM, Kyrill Tkachov <kyrylo.tkac...@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.tkac...@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.tkac...@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.tkac...@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.tkac...@arm.com> >>>>> >>>>> * gcc.dg/20161219.c: New test. >>>> >>>> >
