Re: [PATCH] Fix up handling of bool BIT_NOT_EXPRs in store-merging (PR tree-optimization/84982)

[Richard Biener <rguenther at suse dot de>]

On Tue, 20 Mar 2018, Jakub Jelinek wrote:

> Hi!
> 
> Boolean !x is often expanded as ^ 1, but store merging it actually merges
> as ^ 255 (for 8-bit bool), which is incorrect.
> 
> The following patch fixes it to do that ^ 1 instead.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux and checked on the
> testcase with -> powerpc64-linux cross, ok for trunk?
> 
> 2018-03-20  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 13:53:31.701938584 +0100
> +++ gcc/gimple-ssa-store-merging.c	2018-03-20 14:39:50.925657339 +0100
> @@ -3248,16 +3248,22 @@ invert_op (split_store *split_store, int
>    unsigned int i;
>    store_immediate_info *info;
>    unsigned int cnt = 0;
> +  bool any_bools = 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 (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.

> +	    any_bools = 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_bools)
>      return BIT_NOT_EXPR;
>  
>    unsigned HOST_WIDE_INT try_bitpos = split_store->bytepos * BITS_PER_UNIT;
> @@ -3275,13 +3281,34 @@ invert_op (split_store *split_store, int
>  	 set in the mask.  */
>        unsigned HOST_WIDE_INT bitsize = info->bitsize;
>        unsigned int pos_in_buffer = 0;
> +      bool is_bool = false;
> +      if (any_bools && bitsize > 1)
> +	{
> +	  tree lhs = gimple_assign_lhs (info->stmt);
> +	  if (TREE_CODE (TREE_TYPE (lhs)) == BOOLEAN_TYPE)
> +	    is_bool = true;
> +	}
>        if (info->bitpos < try_bitpos)
>  	{
>  	  gcc_assert (info->bitpos + bitsize > try_bitpos);
>  	  bitsize -= (try_bitpos - info->bitpos);
> +	  if (is_bool && !BYTES_BIG_ENDIAN)
> +	    continue;
>  	}
>        else
>  	pos_in_buffer = info->bitpos - try_bitpos;
> +      if (is_bool && bitsize)
> +	{
> +	  /* If this is a bool inversion, invert just the LSB
> +	     rather than all bits of it.  */
> +	  if (BYTES_BIG_ENDIAN)
> +	    {
> +	      pos_in_buffer += bitsize - 1;
> +	      if (pos_in_buffer >= split_store->size)
> +		continue;
> +	    }
> +	  bitsize = 1;
> +	}
>        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)