* [PATCH] PR86844: Fix for store merging @ 2018-08-07 11:35 Andreas Krebbel 2018-08-17 13:50 ` Richard Biener 0 siblings, 1 reply; 11+ messages in thread From: Andreas Krebbel @ 2018-08-07 11:35 UTC (permalink / raw) To: gcc-patches; +Cc: Andreas Krebbel From: Andreas Krebbel <krebbel@linux.vnet.ibm.com> Bootstrapped and regtested on s390x and x86_64. gcc/ChangeLog: 2018-08-07 Andreas Krebbel <krebbel@linux.ibm.com> PR tree-optimization/86844 * gimple-ssa-store-merging.c (check_no_overlap): Add a check to reject overlaps if it has seen a non-constant store in between. gcc/testsuite/ChangeLog: 2018-08-07 Andreas Krebbel <krebbel@linux.ibm.com> PR tree-optimization/86844 * gcc.dg/pr86844.c: New test. --- gcc/gimple-ssa-store-merging.c | 8 +++++++- gcc/testsuite/gcc.dg/pr86844.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.dg/pr86844.c diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c index 0ae4581..2abef2e 100644 --- a/gcc/gimple-ssa-store-merging.c +++ b/gcc/gimple-ssa-store-merging.c @@ -2401,13 +2401,19 @@ check_no_overlap (vec<store_immediate_info *> m_store_info, unsigned int i, unsigned HOST_WIDE_INT end) { unsigned int len = m_store_info.length (); + bool seen_group_end_store_p = false; + for (++i; i < len; ++i) { store_immediate_info *info = m_store_info[i]; if (info->bitpos >= end) break; + if (info->rhs_code != INTEGER_CST) + seen_group_end_store_p = true; if (info->order < last_order - && (rhs_code != INTEGER_CST || info->rhs_code != INTEGER_CST)) + && (rhs_code != INTEGER_CST + || info->rhs_code != INTEGER_CST + || seen_group_end_store_p)) return false; } return true; diff --git a/gcc/testsuite/gcc.dg/pr86844.c b/gcc/testsuite/gcc.dg/pr86844.c new file mode 100644 index 0000000..9ef08e9 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr86844.c @@ -0,0 +1,42 @@ +/* { dg-do run } */ +/* { dg-require-effective-target stdint_types } */ +/* { dg-options "-O1 -fstore-merging" } */ + +#include <stdint.h> + +struct foo +{ + union + { + uint32_t u4i; + + struct + { + uint8_t x; + uint8_t y; + uint8_t z; + uint8_t w; + } s; + } u; + uint8_t v; +}; + +void __attribute__((noinline,noclone)) +f (struct foo *a) +{ + a->u.u4i = 0; + a->u.s.w = 222; + a->u.s.y = 129; + a->u.s.z = a->v; +} + +int +main () +{ + struct foo s; + + f (&s); + + if (s.u.s.w != 222) + __builtin_abort (); +} -- 2.9.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PR86844: Fix for store merging 2018-08-07 11:35 [PATCH] PR86844: Fix for store merging Andreas Krebbel @ 2018-08-17 13:50 ` Richard Biener 2018-08-18 9:20 ` Eric Botcazou 0 siblings, 1 reply; 11+ messages in thread From: Richard Biener @ 2018-08-17 13:50 UTC (permalink / raw) To: krebbel, Eric Botcazou; +Cc: GCC Patches, Andreas Krebbel On Tue, Aug 7, 2018 at 1:35 PM Andreas Krebbel <krebbel@linux.ibm.com> wrote: > > From: Andreas Krebbel <krebbel@linux.vnet.ibm.com> > > Bootstrapped and regtested on s390x and x86_64. Eric, didn't your patches explicitely handle this case of a non-constant inbetween? Can you have a look / review here? Thanks, Richard. > gcc/ChangeLog: > > 2018-08-07 Andreas Krebbel <krebbel@linux.ibm.com> > > PR tree-optimization/86844 > * gimple-ssa-store-merging.c (check_no_overlap): Add a check to > reject overlaps if it has seen a non-constant store in between. > > gcc/testsuite/ChangeLog: > > 2018-08-07 Andreas Krebbel <krebbel@linux.ibm.com> > > PR tree-optimization/86844 > * gcc.dg/pr86844.c: New test. > --- > gcc/gimple-ssa-store-merging.c | 8 +++++++- > gcc/testsuite/gcc.dg/pr86844.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 49 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.dg/pr86844.c > > diff --git a/gcc/gimple-ssa-store-merging.c b/gcc/gimple-ssa-store-merging.c > index 0ae4581..2abef2e 100644 > --- a/gcc/gimple-ssa-store-merging.c > +++ b/gcc/gimple-ssa-store-merging.c > @@ -2401,13 +2401,19 @@ check_no_overlap (vec<store_immediate_info *> m_store_info, unsigned int i, > unsigned HOST_WIDE_INT end) > { > unsigned int len = m_store_info.length (); > + bool seen_group_end_store_p = false; > + > for (++i; i < len; ++i) > { > store_immediate_info *info = m_store_info[i]; > if (info->bitpos >= end) > break; > + if (info->rhs_code != INTEGER_CST) > + seen_group_end_store_p = true; > if (info->order < last_order > - && (rhs_code != INTEGER_CST || info->rhs_code != INTEGER_CST)) > + && (rhs_code != INTEGER_CST > + || info->rhs_code != INTEGER_CST > + || seen_group_end_store_p)) > return false; > } > return true; > diff --git a/gcc/testsuite/gcc.dg/pr86844.c b/gcc/testsuite/gcc.dg/pr86844.c > new file mode 100644 > index 0000000..9ef08e9 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr86844.c > @@ -0,0 +1,42 @@ > +/* { dg-do run } */ > +/* { dg-require-effective-target stdint_types } */ > +/* { dg-options "-O1 -fstore-merging" } */ > + > +#include <stdint.h> > + > +struct foo > +{ > + union > + { > + uint32_t u4i; > + > + struct > + { > + uint8_t x; > + uint8_t y; > + uint8_t z; > + uint8_t w; > + } s; > + } u; > + uint8_t v; > +}; > + > +void __attribute__((noinline,noclone)) > +f (struct foo *a) > +{ > + a->u.u4i = 0; > + a->u.s.w = 222; > + a->u.s.y = 129; > + a->u.s.z = a->v; > +} > + > +int > +main () > +{ > + struct foo s; > + > + f (&s); > + > + if (s.u.s.w != 222) > + __builtin_abort (); > +} > -- > 2.9.1 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PR86844: Fix for store merging 2018-08-17 13:50 ` Richard Biener @ 2018-08-18 9:20 ` Eric Botcazou 2018-08-20 14:30 ` Jeff Law 0 siblings, 1 reply; 11+ messages in thread From: Eric Botcazou @ 2018-08-18 9:20 UTC (permalink / raw) To: Richard Biener; +Cc: krebbel, GCC Patches, Andreas Krebbel, Jakub Jelinek > Eric, didn't your patches explicitely handle this case of a non-constant > inbetween? Only if there is no overlap at all, otherwise you cannot do things simply. > Can you have a look / review here? Jakub is probably more qualified to give a definitive opinion, as he wrote check_no_overlap and the bug is orthogonal to my patches since it is present in 8.x; in any case, all transformations are supposed to be covered by the testsuite. -- Eric Botcazou ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PR86844: Fix for store merging 2018-08-18 9:20 ` Eric Botcazou @ 2018-08-20 14:30 ` Jeff Law 2018-09-10 14:06 ` Andreas Krebbel 0 siblings, 1 reply; 11+ messages in thread From: Jeff Law @ 2018-08-20 14:30 UTC (permalink / raw) To: Eric Botcazou, Richard Biener Cc: krebbel, GCC Patches, Andreas Krebbel, Jakub Jelinek On 08/18/2018 03:20 AM, Eric Botcazou wrote: >> Eric, didn't your patches explicitely handle this case of a non-constant >> inbetween? > > Only if there is no overlap at all, otherwise you cannot do things simply. > >> Can you have a look / review here? > > Jakub is probably more qualified to give a definitive opinion, as he wrote > check_no_overlap and the bug is orthogonal to my patches since it is present > in 8.x; in any case, all transformations are supposed to be covered by the > testsuite. FYI. Jakub is on PTO through the end of this week and will probably be buried when he returns. Jeff ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PR86844: Fix for store merging 2018-08-20 14:30 ` Jeff Law @ 2018-09-10 14:06 ` Andreas Krebbel 2018-09-10 17:53 ` Jakub Jelinek 0 siblings, 1 reply; 11+ messages in thread From: Andreas Krebbel @ 2018-09-10 14:06 UTC (permalink / raw) To: Jeff Law, Eric Botcazou, Richard Biener, Jakub Jelinek Cc: GCC Patches, Andreas Krebbel On 20.08.2018 16:30, Jeff Law wrote: > On 08/18/2018 03:20 AM, Eric Botcazou wrote: >>> Eric, didn't your patches explicitely handle this case of a non-constant >>> inbetween? >> >> Only if there is no overlap at all, otherwise you cannot do things simply. >> >>> Can you have a look / review here? >> >> Jakub is probably more qualified to give a definitive opinion, as he wrote >> check_no_overlap and the bug is orthogonal to my patches since it is present >> in 8.x; in any case, all transformations are supposed to be covered by the >> testsuite. > FYI. Jakub is on PTO through the end of this week and will probably be > buried when he returns. Jakub, could you please have a look whether that's the right fix? https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00474.html Andreas ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PR86844: Fix for store merging 2018-09-10 14:06 ` Andreas Krebbel @ 2018-09-10 17:53 ` Jakub Jelinek 2018-09-11 14:06 ` Andreas Krebbel 0 siblings, 1 reply; 11+ messages in thread From: Jakub Jelinek @ 2018-09-10 17:53 UTC (permalink / raw) To: Andreas Krebbel Cc: Jeff Law, Eric Botcazou, Richard Biener, GCC Patches, Andreas Krebbel On Mon, Sep 10, 2018 at 04:05:26PM +0200, Andreas Krebbel wrote: > On 20.08.2018 16:30, Jeff Law wrote: > > On 08/18/2018 03:20 AM, Eric Botcazou wrote: > >>> Eric, didn't your patches explicitely handle this case of a non-constant > >>> inbetween? > >> > >> Only if there is no overlap at all, otherwise you cannot do things simply. > >> > >>> Can you have a look / review here? > >> > >> Jakub is probably more qualified to give a definitive opinion, as he wrote > >> check_no_overlap and the bug is orthogonal to my patches since it is present > >> in 8.x; in any case, all transformations are supposed to be covered by the > >> testsuite. > > FYI. Jakub is on PTO through the end of this week and will probably be > > buried when he returns. > > Jakub, could you please have a look whether that's the right fix? > > https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00474.html It is a fix, but not optimal. We have essentially: MEM[(int *)p_28] = 0; MEM[(char *)p_28 + 3B] = 1; MEM[(char *)p_28 + 1B] = 2; MEM[(char *)p_28 + 2B] = MEM[(char *)p_28 + 6B]; It is useful to merge the first 3 stores into: MEM[(int *)p_28] = 0x01000200; // or 0x00020001; depending on endianity MEM[(char *)p_28 + 2B] = MEM[(char *)p_28 + 6B]; rather than punt, and just ignore (i.e. make sure it isn't merged with anything else) the non-INTEGER_CST store). If you don't mind, I'll take this PR over and handle it tomorrow. Slightly tweaked testcase: __attribute__((noipa)) void foo (int *p) { *p = 0; *((char *)p + 3) = 1; *((char *)p + 1) = 2; *((char *)p + 2) = *((char *)p + 6); } int main () { int a[2] = { -1, 0 }; if (sizeof (int) != 4) return 0; ((char *)a)[6] = 3; foo (a); if (((char *)a)[0] != 0 || ((char *)a)[1] != 2 || ((char *)a)[2] != 3 || ((char *)a)[3] != 1) __builtin_abort (); } Jakub ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] PR86844: Fix for store merging 2018-09-10 17:53 ` Jakub Jelinek @ 2018-09-11 14:06 ` Andreas Krebbel 2018-09-12 8:54 ` [PATCH] Fix store merging (PR tree-optimization/86844) Jakub Jelinek 0 siblings, 1 reply; 11+ messages in thread From: Andreas Krebbel @ 2018-09-11 14:06 UTC (permalink / raw) To: Jakub Jelinek Cc: Jeff Law, Eric Botcazou, Richard Biener, GCC Patches, Andreas Krebbel On 10.09.2018 19:53, Jakub Jelinek wrote: > On Mon, Sep 10, 2018 at 04:05:26PM +0200, Andreas Krebbel wrote: >> On 20.08.2018 16:30, Jeff Law wrote: >>> On 08/18/2018 03:20 AM, Eric Botcazou wrote: >>>>> Eric, didn't your patches explicitely handle this case of a non-constant >>>>> inbetween? >>>> >>>> Only if there is no overlap at all, otherwise you cannot do things simply. >>>> >>>>> Can you have a look / review here? >>>> >>>> Jakub is probably more qualified to give a definitive opinion, as he wrote >>>> check_no_overlap and the bug is orthogonal to my patches since it is present >>>> in 8.x; in any case, all transformations are supposed to be covered by the >>>> testsuite. >>> FYI. Jakub is on PTO through the end of this week and will probably be >>> buried when he returns. >> >> Jakub, could you please have a look whether that's the right fix? >> >> https://gcc.gnu.org/ml/gcc-patches/2018-08/msg00474.html > > It is a fix, but not optimal. > We have essentially: > MEM[(int *)p_28] = 0; > MEM[(char *)p_28 + 3B] = 1; > MEM[(char *)p_28 + 1B] = 2; > MEM[(char *)p_28 + 2B] = MEM[(char *)p_28 + 6B]; > It is useful to merge the first 3 stores into: > MEM[(int *)p_28] = 0x01000200; // or 0x00020001; depending on endianity > MEM[(char *)p_28 + 2B] = MEM[(char *)p_28 + 6B]; > rather than punt, and just ignore (i.e. make sure it isn't merged with > anything else) the non-INTEGER_CST store). If you don't mind, I'll take this > PR over and handle it tomorrow. Please do. Thanks! Andreas > > Slightly tweaked testcase: > __attribute__((noipa)) void > foo (int *p) > { > *p = 0; > *((char *)p + 3) = 1; > *((char *)p + 1) = 2; > *((char *)p + 2) = *((char *)p + 6); > } > > int > main () > { > int a[2] = { -1, 0 }; > if (sizeof (int) != 4) > return 0; > ((char *)a)[6] = 3; > foo (a); > if (((char *)a)[0] != 0 || ((char *)a)[1] != 2 > || ((char *)a)[2] != 3 || ((char *)a)[3] != 1) > __builtin_abort (); > } > > Jakub > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] Fix store merging (PR tree-optimization/86844) 2018-09-11 14:06 ` Andreas Krebbel @ 2018-09-12 8:54 ` Jakub Jelinek 2018-09-12 9:04 ` Richard Biener 0 siblings, 1 reply; 11+ messages in thread From: Jakub Jelinek @ 2018-09-12 8:54 UTC (permalink / raw) To: Richard Biener, Andreas Krebbel; +Cc: Jeff Law, Eric Botcazou, GCC Patches On Tue, Sep 11, 2018 at 04:06:40PM +0200, Andreas Krebbel wrote: > > It is a fix, but not optimal. > > We have essentially: > > MEM[(int *)p_28] = 0; > > MEM[(char *)p_28 + 3B] = 1; > > MEM[(char *)p_28 + 1B] = 2; > > MEM[(char *)p_28 + 2B] = MEM[(char *)p_28 + 6B]; > > It is useful to merge the first 3 stores into: > > MEM[(int *)p_28] = 0x01000200; // or 0x00020001; depending on endianity > > MEM[(char *)p_28 + 2B] = MEM[(char *)p_28 + 6B]; > > rather than punt, and just ignore (i.e. make sure it isn't merged with > > anything else) the non-INTEGER_CST store). If you don't mind, I'll take this > > PR over and handle it tomorrow. > > Please do. Thanks! Here it is, I hope the added comments are clear enough on what's going on and when we can and when we can't handle it. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk and eventually 8.3? 2018-09-12 Jakub Jelinek <jakub@redhat.com> Andreas Krebbel <krebbel@linux.ibm.com> PR tree-optimization/86844 * gimple-ssa-store-merging.c (imm_store_chain_info::coalesce_immediate): For overlapping stores, if there are any overlapping stores in between them, make sure they are also coalesced or we give up completely. * gcc.c-torture/execute/pr86844.c: New test. * gcc.dg/store_merging_22.c: New test. * gcc.dg/store_merging_23.c: New test. --- gcc/gimple-ssa-store-merging.c.jj 2018-07-12 09:38:46.137335036 +0200 +++ gcc/gimple-ssa-store-merging.c 2018-09-11 22:47:41.406582798 +0200 @@ -2702,15 +2702,80 @@ imm_store_chain_info::coalesce_immediate { /* Only allow overlapping stores of constants. */ if (info->rhs_code == INTEGER_CST - && merged_store->stores[0]->rhs_code == INTEGER_CST - && check_no_overlap (m_store_info, i, INTEGER_CST, - MAX (merged_store->last_order, info->order), - MAX (merged_store->start - + merged_store->width, - info->bitpos + info->bitsize))) + && merged_store->stores[0]->rhs_code == INTEGER_CST) { - merged_store->merge_overlapping (info); - goto done; + unsigned int last_order + = MAX (merged_store->last_order, info->order); + unsigned HOST_WIDE_INT end + = MAX (merged_store->start + merged_store->width, + info->bitpos + info->bitsize); + if (check_no_overlap (m_store_info, i, INTEGER_CST, + last_order, end)) + { + /* check_no_overlap call above made sure there are no + overlapping stores with non-INTEGER_CST rhs_code + in between the first and last of the stores we've + just merged. If there are any INTEGER_CST rhs_code + stores in between, we need to merge_overlapping them + even if in the sort_by_bitpos order there are other + overlapping stores in between. Keep those stores as is. + Example: + MEM[(int *)p_28] = 0; + MEM[(char *)p_28 + 3B] = 1; + MEM[(char *)p_28 + 1B] = 2; + MEM[(char *)p_28 + 2B] = MEM[(char *)p_28 + 6B]; + We can't merge the zero store with the store of two and + not merge anything else, because the store of one is + in the original order in between those two, but in + store_by_bitpos order it comes after the last store that + we can't merge with them. We can merge the first 3 stores + and keep the last store as is though. */ + unsigned int len = m_store_info.length (), k = i; + for (unsigned int j = i + 1; j < len; ++j) + { + store_immediate_info *info2 = m_store_info[j]; + if (info2->bitpos >= end) + break; + if (info2->order < last_order) + { + if (info2->rhs_code != INTEGER_CST) + { + /* Normally check_no_overlap makes sure this + doesn't happen, but if end grows below, then + we need to process more stores than + check_no_overlap verified. Example: + MEM[(int *)p_5] = 0; + MEM[(short *)p_5 + 3B] = 1; + MEM[(char *)p_5 + 4B] = _9; + MEM[(char *)p_5 + 2B] = 2; */ + k = 0; + break; + } + k = j; + end = MAX (end, info2->bitpos + info2->bitsize); + } + } + + if (k != 0) + { + merged_store->merge_overlapping (info); + + for (unsigned int j = i + 1; j <= k; j++) + { + store_immediate_info *info2 = m_store_info[j]; + gcc_assert (info2->bitpos < end); + if (info2->order < last_order) + { + gcc_assert (info2->rhs_code == INTEGER_CST); + merged_store->merge_overlapping (info2); + } + /* Other stores are kept and not merged in any + way. */ + } + ignore = k; + goto done; + } + } } } /* |---store 1---||---store 2---| --- gcc/testsuite/gcc.c-torture/execute/pr86844.c.jj 2018-09-11 18:25:16.882452028 +0200 +++ gcc/testsuite/gcc.c-torture/execute/pr86844.c 2018-09-11 18:25:16.882452028 +0200 @@ -0,0 +1,24 @@ +/* PR tree-optimization/86844 */ + +__attribute__((noipa)) void +foo (int *p) +{ + *p = 0; + *((char *)p + 3) = 1; + *((char *)p + 1) = 2; + *((char *)p + 2) = *((char *)p + 6); +} + +int +main () +{ + int a[2] = { -1, 0 }; + if (sizeof (int) != 4) + return 0; + ((char *)a)[6] = 3; + foo (a); + if (((char *)a)[0] != 0 || ((char *)a)[1] != 2 + || ((char *)a)[2] != 3 || ((char *)a)[3] != 1) + __builtin_abort (); + return 0; +} --- gcc/testsuite/gcc.dg/store_merging_22.c.jj 2018-09-12 10:14:34.286704682 +0200 +++ gcc/testsuite/gcc.dg/store_merging_22.c 2018-09-12 10:25:30.156727153 +0200 @@ -0,0 +1,16 @@ +/* PR tree-optimization/86844 */ +/* { dg-do compile } */ +/* { dg-require-effective-target store_merge } */ +/* { dg-options "-O2 -fdump-tree-store-merging" } */ + +void +foo (int *p) +{ + *p = 0; + *((char *)p + 3) = 1; + *((char *)p + 1) = 2; + *((char *)p + 2) = *((char *)p + 38); +} + +/* { dg-final { scan-tree-dump-times "Merging successful" 1 "store-merging" } } */ +/* { dg-final { scan-tree-dump-times "New sequence of 1 stores to replace old one of 3 stores" 1 "store-merging" } } */ --- gcc/testsuite/gcc.dg/store_merging_23.c.jj 2018-09-12 10:25:41.099544995 +0200 +++ gcc/testsuite/gcc.dg/store_merging_23.c 2018-09-12 10:32:27.956772277 +0200 @@ -0,0 +1,16 @@ +/* PR tree-optimization/86844 */ +/* { dg-do compile } */ +/* { dg-require-effective-target store_merge } */ +/* { dg-options "-O2 -fdump-tree-store-merging" } */ + +void +foo (int *p) +{ + *p = 0; + int one = 1; + __builtin_memcpy ((char *) p + 3, &one, sizeof (int)); + *((char *)p + 4) = *((char *)p + 36); + *((char *)p + 1) = 2; +} + +/* { dg-final { scan-tree-dump-not "Merging successful" "store-merging" } } */ Jakub ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix store merging (PR tree-optimization/86844) 2018-09-12 8:54 ` [PATCH] Fix store merging (PR tree-optimization/86844) Jakub Jelinek @ 2018-09-12 9:04 ` Richard Biener 2018-09-12 9:11 ` Jakub Jelinek 0 siblings, 1 reply; 11+ messages in thread From: Richard Biener @ 2018-09-12 9:04 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Andreas Krebbel, Jeff Law, Eric Botcazou, GCC Patches On Wed, 12 Sep 2018, Jakub Jelinek wrote: > On Tue, Sep 11, 2018 at 04:06:40PM +0200, Andreas Krebbel wrote: > > > It is a fix, but not optimal. > > > We have essentially: > > > MEM[(int *)p_28] = 0; > > > MEM[(char *)p_28 + 3B] = 1; > > > MEM[(char *)p_28 + 1B] = 2; > > > MEM[(char *)p_28 + 2B] = MEM[(char *)p_28 + 6B]; > > > It is useful to merge the first 3 stores into: > > > MEM[(int *)p_28] = 0x01000200; // or 0x00020001; depending on endianity > > > MEM[(char *)p_28 + 2B] = MEM[(char *)p_28 + 6B]; > > > rather than punt, and just ignore (i.e. make sure it isn't merged with > > > anything else) the non-INTEGER_CST store). If you don't mind, I'll take this > > > PR over and handle it tomorrow. > > > > Please do. Thanks! > > Here it is, I hope the added comments are clear enough on what's going on > and when we can and when we can't handle it. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk and > eventually 8.3? OK. We need to fix the bug on the branch, so either we go with this variant or the other (which was pessimizing some cases?) Richard. > 2018-09-12 Jakub Jelinek <jakub@redhat.com> > Andreas Krebbel <krebbel@linux.ibm.com> > > PR tree-optimization/86844 > * gimple-ssa-store-merging.c > (imm_store_chain_info::coalesce_immediate): For overlapping stores, if > there are any overlapping stores in between them, make sure they are > also coalesced or we give up completely. > > * gcc.c-torture/execute/pr86844.c: New test. > * gcc.dg/store_merging_22.c: New test. > * gcc.dg/store_merging_23.c: New test. > > --- gcc/gimple-ssa-store-merging.c.jj 2018-07-12 09:38:46.137335036 +0200 > +++ gcc/gimple-ssa-store-merging.c 2018-09-11 22:47:41.406582798 +0200 > @@ -2702,15 +2702,80 @@ imm_store_chain_info::coalesce_immediate > { > /* Only allow overlapping stores of constants. */ > if (info->rhs_code == INTEGER_CST > - && merged_store->stores[0]->rhs_code == INTEGER_CST > - && check_no_overlap (m_store_info, i, INTEGER_CST, > - MAX (merged_store->last_order, info->order), > - MAX (merged_store->start > - + merged_store->width, > - info->bitpos + info->bitsize))) > + && merged_store->stores[0]->rhs_code == INTEGER_CST) > { > - merged_store->merge_overlapping (info); > - goto done; > + unsigned int last_order > + = MAX (merged_store->last_order, info->order); > + unsigned HOST_WIDE_INT end > + = MAX (merged_store->start + merged_store->width, > + info->bitpos + info->bitsize); > + if (check_no_overlap (m_store_info, i, INTEGER_CST, > + last_order, end)) > + { > + /* check_no_overlap call above made sure there are no > + overlapping stores with non-INTEGER_CST rhs_code > + in between the first and last of the stores we've > + just merged. If there are any INTEGER_CST rhs_code > + stores in between, we need to merge_overlapping them > + even if in the sort_by_bitpos order there are other > + overlapping stores in between. Keep those stores as is. > + Example: > + MEM[(int *)p_28] = 0; > + MEM[(char *)p_28 + 3B] = 1; > + MEM[(char *)p_28 + 1B] = 2; > + MEM[(char *)p_28 + 2B] = MEM[(char *)p_28 + 6B]; > + We can't merge the zero store with the store of two and > + not merge anything else, because the store of one is > + in the original order in between those two, but in > + store_by_bitpos order it comes after the last store that > + we can't merge with them. We can merge the first 3 stores > + and keep the last store as is though. */ > + unsigned int len = m_store_info.length (), k = i; > + for (unsigned int j = i + 1; j < len; ++j) > + { > + store_immediate_info *info2 = m_store_info[j]; > + if (info2->bitpos >= end) > + break; > + if (info2->order < last_order) > + { > + if (info2->rhs_code != INTEGER_CST) > + { > + /* Normally check_no_overlap makes sure this > + doesn't happen, but if end grows below, then > + we need to process more stores than > + check_no_overlap verified. Example: > + MEM[(int *)p_5] = 0; > + MEM[(short *)p_5 + 3B] = 1; > + MEM[(char *)p_5 + 4B] = _9; > + MEM[(char *)p_5 + 2B] = 2; */ > + k = 0; > + break; > + } > + k = j; > + end = MAX (end, info2->bitpos + info2->bitsize); > + } > + } > + > + if (k != 0) > + { > + merged_store->merge_overlapping (info); > + > + for (unsigned int j = i + 1; j <= k; j++) > + { > + store_immediate_info *info2 = m_store_info[j]; > + gcc_assert (info2->bitpos < end); > + if (info2->order < last_order) > + { > + gcc_assert (info2->rhs_code == INTEGER_CST); > + merged_store->merge_overlapping (info2); > + } > + /* Other stores are kept and not merged in any > + way. */ > + } > + ignore = k; > + goto done; > + } > + } > } > } > /* |---store 1---||---store 2---| > --- gcc/testsuite/gcc.c-torture/execute/pr86844.c.jj 2018-09-11 18:25:16.882452028 +0200 > +++ gcc/testsuite/gcc.c-torture/execute/pr86844.c 2018-09-11 18:25:16.882452028 +0200 > @@ -0,0 +1,24 @@ > +/* PR tree-optimization/86844 */ > + > +__attribute__((noipa)) void > +foo (int *p) > +{ > + *p = 0; > + *((char *)p + 3) = 1; > + *((char *)p + 1) = 2; > + *((char *)p + 2) = *((char *)p + 6); > +} > + > +int > +main () > +{ > + int a[2] = { -1, 0 }; > + if (sizeof (int) != 4) > + return 0; > + ((char *)a)[6] = 3; > + foo (a); > + if (((char *)a)[0] != 0 || ((char *)a)[1] != 2 > + || ((char *)a)[2] != 3 || ((char *)a)[3] != 1) > + __builtin_abort (); > + return 0; > +} > --- gcc/testsuite/gcc.dg/store_merging_22.c.jj 2018-09-12 10:14:34.286704682 +0200 > +++ gcc/testsuite/gcc.dg/store_merging_22.c 2018-09-12 10:25:30.156727153 +0200 > @@ -0,0 +1,16 @@ > +/* PR tree-optimization/86844 */ > +/* { dg-do compile } */ > +/* { dg-require-effective-target store_merge } */ > +/* { dg-options "-O2 -fdump-tree-store-merging" } */ > + > +void > +foo (int *p) > +{ > + *p = 0; > + *((char *)p + 3) = 1; > + *((char *)p + 1) = 2; > + *((char *)p + 2) = *((char *)p + 38); > +} > + > +/* { dg-final { scan-tree-dump-times "Merging successful" 1 "store-merging" } } */ > +/* { dg-final { scan-tree-dump-times "New sequence of 1 stores to replace old one of 3 stores" 1 "store-merging" } } */ > --- gcc/testsuite/gcc.dg/store_merging_23.c.jj 2018-09-12 10:25:41.099544995 +0200 > +++ gcc/testsuite/gcc.dg/store_merging_23.c 2018-09-12 10:32:27.956772277 +0200 > @@ -0,0 +1,16 @@ > +/* PR tree-optimization/86844 */ > +/* { dg-do compile } */ > +/* { dg-require-effective-target store_merge } */ > +/* { dg-options "-O2 -fdump-tree-store-merging" } */ > + > +void > +foo (int *p) > +{ > + *p = 0; > + int one = 1; > + __builtin_memcpy ((char *) p + 3, &one, sizeof (int)); > + *((char *)p + 4) = *((char *)p + 36); > + *((char *)p + 1) = 2; > +} > + > +/* { dg-final { scan-tree-dump-not "Merging successful" "store-merging" } } */ > > > Jakub > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix store merging (PR tree-optimization/86844) 2018-09-12 9:04 ` Richard Biener @ 2018-09-12 9:11 ` Jakub Jelinek 2018-09-12 11:02 ` Richard Biener 0 siblings, 1 reply; 11+ messages in thread From: Jakub Jelinek @ 2018-09-12 9:11 UTC (permalink / raw) To: Richard Biener; +Cc: Andreas Krebbel, Jeff Law, Eric Botcazou, GCC Patches On Wed, Sep 12, 2018 at 11:04:52AM +0200, Richard Biener wrote: > OK. We need to fix the bug on the branch, so either we go with this > variant or the other (which was pessimizing some cases?) Yeah. Andreas' patch is simpler and for that reason might be better to backport, on the other side trying to keep trunk and branch in sync has advantages as well, especially for potential backports of further store-merging fixes if they'll be needed. Jakub ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] Fix store merging (PR tree-optimization/86844) 2018-09-12 9:11 ` Jakub Jelinek @ 2018-09-12 11:02 ` Richard Biener 0 siblings, 0 replies; 11+ messages in thread From: Richard Biener @ 2018-09-12 11:02 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Andreas Krebbel, Jeff Law, Eric Botcazou, GCC Patches On Wed, 12 Sep 2018, Jakub Jelinek wrote: > On Wed, Sep 12, 2018 at 11:04:52AM +0200, Richard Biener wrote: > > OK. We need to fix the bug on the branch, so either we go with this > > variant or the other (which was pessimizing some cases?) > > Yeah. Andreas' patch is simpler and for that reason might be better to > backport, on the other side trying to keep trunk and branch in sync has > advantages as well, especially for potential backports of further > store-merging fixes if they'll be needed. I'd say backport the change if no issues show up within a week. Richard. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-09-12 11:02 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-07 11:35 [PATCH] PR86844: Fix for store merging Andreas Krebbel 2018-08-17 13:50 ` Richard Biener 2018-08-18 9:20 ` Eric Botcazou 2018-08-20 14:30 ` Jeff Law 2018-09-10 14:06 ` Andreas Krebbel 2018-09-10 17:53 ` Jakub Jelinek 2018-09-11 14:06 ` Andreas Krebbel 2018-09-12 8:54 ` [PATCH] Fix store merging (PR tree-optimization/86844) Jakub Jelinek 2018-09-12 9:04 ` Richard Biener 2018-09-12 9:11 ` Jakub Jelinek 2018-09-12 11:02 ` Richard Biener
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).