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

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