| Message ID | 586FC9BA.7020608@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
On January 6, 2017 5:45:46 PM GMT+01:00, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote: > >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? OK. Richard. >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. >>>>>>> >>
diff --git a/gcc/testsuite/gcc.dg/20161219.c b/gcc/testsuite/gcc.dg/20161219.c new file mode 100644 index 0000000000000000000000000000000000000000..93ea8d2364d9ab54704a84e6c0bff0427df82db8 --- /dev/null +++ b/gcc/testsuite/gcc.dg/20161219.c @@ -0,0 +1,30 @@ +/* { dg-do assemble } */ +/* { dg-options "-O1 -w" } */ + +static long long a[9]; +int b, c, d, e, g; + +static int +fn1 (int *p1) +{ + b = 1; + for (; b >= 0; b--) + { + d = 0; + for (;; d++) + { + e && (a[d] = 0); + if (*p1) + break; + c = (int) a; + } + } + return 0; +} + +int +main () +{ + int f = fn1 ((int *) f); + return f; +} diff --git a/gcc/tree-ssa-address.c b/gcc/tree-ssa-address.c index 3e3cad150b64e813509e079f9ea91d65806e414a..8d46a3e67337dd7639d0b17ca888f50009d65b93 100644 --- a/gcc/tree-ssa-address.c +++ b/gcc/tree-ssa-address.c @@ -115,7 +115,7 @@ gen_addr_rtx (machine_mode address_mode, if (offset_p) *offset_p = NULL; - if (index) + if (index && index != const0_rtx) { act_elem = index; if (step)