| Message ID | 585955CF.3010501@foss.arm.com |
|---|---|
| State | Superseded |
| Headers | show |
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? >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.
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? 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. >
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. >> >> >
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) ? 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. 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. >>>
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. >>>> >>>> >
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. >> 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 a53ade0600d01c9d48f6c789b78fd42d7a06a95b..0c7c5902cebb0196c8e27f4eba45ce176cc3f0a5 100644 --- a/gcc/tree-ssa-address.c +++ b/gcc/tree-ssa-address.c @@ -154,7 +154,7 @@ gen_addr_rtx (machine_mode address_mode, } if (*addr) - *addr = gen_rtx_PLUS (address_mode, *addr, act_elem); + *addr = simplify_gen_binary (PLUS, address_mode, *addr, act_elem); else *addr = act_elem; }
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. 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.