PR86844: Fix for store merging

Message ID 20180807113526.15097-1-krebbel@linux.ibm.com
State New
Headers show
Series
  • PR86844: Fix for store merging
Related show

Commit Message

Andreas Krebbel Aug. 7, 2018, 11:35 a.m.
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

Comments

Richard Biener Aug. 17, 2018, 1:50 p.m. | #1
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 Botcazou Aug. 18, 2018, 9:20 a.m. | #2
> 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.
Jeff Law Aug. 20, 2018, 2:30 p.m. | #3
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
Andreas Krebbel Sept. 10, 2018, 2:05 p.m. | #4
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
Jakub Jelinek Sept. 10, 2018, 5:53 p.m. | #5
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
Andreas Krebbel Sept. 11, 2018, 2:06 p.m. | #6
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
>

Patch

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 ();
+}