Fix up handling of bool BIT_NOT_EXPRs in store-merging (PR tree-optimization/84982, take 2)

Message ID 20180321100710.GH8577@tucnak
State New
Headers show
Series
  • Fix up handling of bool BIT_NOT_EXPRs in store-merging (PR tree-optimization/84982, take 2)
Related show

Commit Message

Jakub Jelinek March 21, 2018, 10:07 a.m.
On Wed, Mar 21, 2018 at 09:20:40AM +0100, Richard Biener wrote:
> > +	  tree lhs = gimple_assign_lhs (info->stmt);

> > +	  if (TREE_CODE (TREE_TYPE (lhs)) == BOOLEAN_TYPE && info->bitsize > 1)

> 

> So I'm slightly uncomfortable with keying this just on BOOLEAN_TYPE.

> Do you think anything would go wrong with simply using

> 

>  if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))

>      && TYPE_PRECISION (TREE_TYPE (lhs)) < info->bitsize)

> 

> ?

> 

> It would then be any_padding rather than any_bool.


So like this?  The assembly for the testcase is still identical to previous
patch on both x86_64-linux and powerpc-linux.
I've tried to reproduce the case with non-bool integral types with precision
smaller than size, but even with C++ -fstrict-enums haven't succeeded, those
enums have different TYPE_*_VALUE, but TYPE_PRECISION is still equal to the
TYPE_SIZE, in the end I've just changed in gdb TYPE_PRECISION of the
enumerated type and checked that there is (without store merging) xor with
the mask of only precision bits emitted.  Perhaps in Ada one can construct
something?  I don't speak Ada though...

2018-03-21  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/84982
	* gimple-ssa-store-merging.c (invert_op): Handle boolean inversion
	by flipping the least significant bit rather than all bits from
	bitpos to bitpos + bitsize - 1.

	* c-c++-common/pr84982.c: New test.



	Jakub

Comments

Richard Biener March 21, 2018, 10:10 a.m. | #1
On Wed, 21 Mar 2018, Jakub Jelinek wrote:

> On Wed, Mar 21, 2018 at 09:20:40AM +0100, Richard Biener wrote:

> > > +	  tree lhs = gimple_assign_lhs (info->stmt);

> > > +	  if (TREE_CODE (TREE_TYPE (lhs)) == BOOLEAN_TYPE && info->bitsize > 1)

> > 

> > So I'm slightly uncomfortable with keying this just on BOOLEAN_TYPE.

> > Do you think anything would go wrong with simply using

> > 

> >  if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))

> >      && TYPE_PRECISION (TREE_TYPE (lhs)) < info->bitsize)

> > 

> > ?

> > 

> > It would then be any_padding rather than any_bool.

> 

> So like this?  The assembly for the testcase is still identical to previous

> patch on both x86_64-linux and powerpc-linux.


Yes.

> I've tried to reproduce the case with non-bool integral types with precision

> smaller than size, but even with C++ -fstrict-enums haven't succeeded, those

> enums have different TYPE_*_VALUE, but TYPE_PRECISION is still equal to the

> TYPE_SIZE, in the end I've just changed in gdb TYPE_PRECISION of the

> enumerated type and checked that there is (without store merging) xor with

> the mask of only precision bits emitted.  Perhaps in Ada one can construct

> something?  I don't speak Ada though...


Yeah, I'm just fearing there are no rules prohibiting such types ;)
Like for C++ and -fstrict-enums we have constrained TYPE_MIN/MAX_VALUE
but not TYPE_PRECISION (for whatever reason ...).

Patch is ok.

Richard.

> 2018-03-21  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR tree-optimization/84982

> 	* gimple-ssa-store-merging.c (invert_op): Handle boolean inversion

> 	by flipping the least significant bit rather than all bits from

> 	bitpos to bitpos + bitsize - 1.

> 

> 	* c-c++-common/pr84982.c: New test.

> 

> --- gcc/gimple-ssa-store-merging.c.jj	2018-03-20 22:05:54.368430762 +0100

> +++ gcc/gimple-ssa-store-merging.c	2018-03-21 10:45:39.919458647 +0100

> @@ -3248,16 +3248,23 @@ invert_op (split_store *split_store, int

>    unsigned int i;

>    store_immediate_info *info;

>    unsigned int cnt = 0;

> +  bool any_paddings = false;

>    FOR_EACH_VEC_ELT (split_store->orig_stores, i, info)

>      {

>        bool bit_not_p = idx < 2 ? info->ops[idx].bit_not_p : info->bit_not_p;

>        if (bit_not_p)

> -	++cnt;

> +	{

> +	  ++cnt;

> +	  tree lhs = gimple_assign_lhs (info->stmt);

> +	  if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))

> +	      && TYPE_PRECISION (TREE_TYPE (lhs)) < info->bitsize)

> +	    any_paddings = true;

> +	}

>      }

>    mask = NULL_TREE;

>    if (cnt == 0)

>      return NOP_EXPR;

> -  if (cnt == split_store->orig_stores.length ())

> +  if (cnt == split_store->orig_stores.length () && !any_paddings)

>      return BIT_NOT_EXPR;

>  

>    unsigned HOST_WIDE_INT try_bitpos = split_store->bytepos * BITS_PER_UNIT;

> @@ -3274,14 +3281,42 @@ invert_op (split_store *split_store, int

>  	 clear regions with !bit_not_p, so that gaps in between stores aren't

>  	 set in the mask.  */

>        unsigned HOST_WIDE_INT bitsize = info->bitsize;

> +      unsigned HOST_WIDE_INT prec = bitsize;

>        unsigned int pos_in_buffer = 0;

> +      if (any_paddings)

> +	{

> +	  tree lhs = gimple_assign_lhs (info->stmt);

> +	  if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))

> +	      && TYPE_PRECISION (TREE_TYPE (lhs)) < bitsize)

> +	    prec = TYPE_PRECISION (TREE_TYPE (lhs));

> +	}

>        if (info->bitpos < try_bitpos)

>  	{

>  	  gcc_assert (info->bitpos + bitsize > try_bitpos);

> -	  bitsize -= (try_bitpos - info->bitpos);

> +	  if (!BYTES_BIG_ENDIAN)

> +	    {

> +	      if (prec <= try_bitpos - info->bitpos)

> +		continue;

> +	      prec -= try_bitpos - info->bitpos;

> +	    }

> +	  bitsize -= try_bitpos - info->bitpos;

> +	  if (BYTES_BIG_ENDIAN && prec > bitsize)

> +	    prec = bitsize;

>  	}

>        else

>  	pos_in_buffer = info->bitpos - try_bitpos;

> +      if (prec < bitsize)

> +	{

> +	  /* If this is a bool inversion, invert just the least significant

> +	     prec bits rather than all bits of it.  */

> +	  if (BYTES_BIG_ENDIAN)

> +	    {

> +	      pos_in_buffer += bitsize - prec;

> +	      if (pos_in_buffer >= split_store->size)

> +		continue;

> +	    }

> +	  bitsize = prec;

> +	}

>        if (pos_in_buffer + bitsize > split_store->size)

>  	bitsize = split_store->size - pos_in_buffer;

>        unsigned char *p = buf + (pos_in_buffer / BITS_PER_UNIT);

> --- gcc/testsuite/c-c++-common/pr84982.c.jj	2018-03-20 14:49:00.259744750 +0100

> +++ gcc/testsuite/c-c++-common/pr84982.c	2018-03-20 12:27:34.111363552 +0100

> @@ -0,0 +1,38 @@

> +/* PR tree-optimization/84982 */

> +/* { dg-do run } */

> +/* { dg-options "-O2" } */

> +

> +#ifndef __cplusplus

> +#define bool _Bool

> +#define true 1

> +#define false 0

> +#endif

> +

> +struct S { bool a, b, c, d; };

> +

> +__attribute__((noipa)) void

> +bar (bool *x)

> +{

> +  if (x[0] || !x[1] || !x[2] || x[3])

> +    __builtin_abort ();

> +}

> +

> +__attribute__((noipa)) void

> +foo (struct S *x)

> +{

> +  bool a[4];

> +  a[0] = !x->a;

> +  a[1] = !x->b;

> +  a[2] = x->c;

> +  a[3] = !x->d;

> +  bar (a);

> +} 

> +

> +int

> +main ()

> +{

> +  struct S s;

> +  s.a = true; s.b = false; s.c = true; s.d = true;

> +  foo (&s);

> +  return 0;

> +}

> 

> 

> 	Jakub

> 

> 


-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Patch

--- gcc/gimple-ssa-store-merging.c.jj	2018-03-20 22:05:54.368430762 +0100
+++ gcc/gimple-ssa-store-merging.c	2018-03-21 10:45:39.919458647 +0100
@@ -3248,16 +3248,23 @@  invert_op (split_store *split_store, int
   unsigned int i;
   store_immediate_info *info;
   unsigned int cnt = 0;
+  bool any_paddings = false;
   FOR_EACH_VEC_ELT (split_store->orig_stores, i, info)
     {
       bool bit_not_p = idx < 2 ? info->ops[idx].bit_not_p : info->bit_not_p;
       if (bit_not_p)
-	++cnt;
+	{
+	  ++cnt;
+	  tree lhs = gimple_assign_lhs (info->stmt);
+	  if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+	      && TYPE_PRECISION (TREE_TYPE (lhs)) < info->bitsize)
+	    any_paddings = true;
+	}
     }
   mask = NULL_TREE;
   if (cnt == 0)
     return NOP_EXPR;
-  if (cnt == split_store->orig_stores.length ())
+  if (cnt == split_store->orig_stores.length () && !any_paddings)
     return BIT_NOT_EXPR;
 
   unsigned HOST_WIDE_INT try_bitpos = split_store->bytepos * BITS_PER_UNIT;
@@ -3274,14 +3281,42 @@  invert_op (split_store *split_store, int
 	 clear regions with !bit_not_p, so that gaps in between stores aren't
 	 set in the mask.  */
       unsigned HOST_WIDE_INT bitsize = info->bitsize;
+      unsigned HOST_WIDE_INT prec = bitsize;
       unsigned int pos_in_buffer = 0;
+      if (any_paddings)
+	{
+	  tree lhs = gimple_assign_lhs (info->stmt);
+	  if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))
+	      && TYPE_PRECISION (TREE_TYPE (lhs)) < bitsize)
+	    prec = TYPE_PRECISION (TREE_TYPE (lhs));
+	}
       if (info->bitpos < try_bitpos)
 	{
 	  gcc_assert (info->bitpos + bitsize > try_bitpos);
-	  bitsize -= (try_bitpos - info->bitpos);
+	  if (!BYTES_BIG_ENDIAN)
+	    {
+	      if (prec <= try_bitpos - info->bitpos)
+		continue;
+	      prec -= try_bitpos - info->bitpos;
+	    }
+	  bitsize -= try_bitpos - info->bitpos;
+	  if (BYTES_BIG_ENDIAN && prec > bitsize)
+	    prec = bitsize;
 	}
       else
 	pos_in_buffer = info->bitpos - try_bitpos;
+      if (prec < bitsize)
+	{
+	  /* If this is a bool inversion, invert just the least significant
+	     prec bits rather than all bits of it.  */
+	  if (BYTES_BIG_ENDIAN)
+	    {
+	      pos_in_buffer += bitsize - prec;
+	      if (pos_in_buffer >= split_store->size)
+		continue;
+	    }
+	  bitsize = prec;
+	}
       if (pos_in_buffer + bitsize > split_store->size)
 	bitsize = split_store->size - pos_in_buffer;
       unsigned char *p = buf + (pos_in_buffer / BITS_PER_UNIT);
--- gcc/testsuite/c-c++-common/pr84982.c.jj	2018-03-20 14:49:00.259744750 +0100
+++ gcc/testsuite/c-c++-common/pr84982.c	2018-03-20 12:27:34.111363552 +0100
@@ -0,0 +1,38 @@ 
+/* PR tree-optimization/84982 */
+/* { dg-do run } */
+/* { dg-options "-O2" } */
+
+#ifndef __cplusplus
+#define bool _Bool
+#define true 1
+#define false 0
+#endif
+
+struct S { bool a, b, c, d; };
+
+__attribute__((noipa)) void
+bar (bool *x)
+{
+  if (x[0] || !x[1] || !x[2] || x[3])
+    __builtin_abort ();
+}
+
+__attribute__((noipa)) void
+foo (struct S *x)
+{
+  bool a[4];
+  a[0] = !x->a;
+  a[1] = !x->b;
+  a[2] = x->c;
+  a[3] = !x->d;
+  bar (a);
+} 
+
+int
+main ()
+{
+  struct S s;
+  s.a = true; s.b = false; s.c = true; s.d = true;
+  foo (&s);
+  return 0;
+}