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

-- 
2.9.1

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.

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