| Message ID | 20180807113526.15097-1-krebbel@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | PR86844: Fix for store merging | expand |
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 >
> 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.
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
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
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
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 >
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 (); +}
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