[PATCH] tree-optimization/99919 - bogus uninit diagnostic with bitfield guards

[PATCH] tree-optimization/99919 - bogus uninit diagnostic with bitfield guards

[Richard Biener]

For the testcase in this PR what fold-const.cc optimize_bit_field_compare
does to bitfield against constant compares is confusing the uninit
predicate analysis and it also makes SRA obfuscate instead of optimize
the code.  We've long had the opinion that those optimizations are
premature but we do not have any replacement for the more complicated
ones combining multiple bitfield tests.  The following disables mangling
the case of a single bitfield test against constants but preserving
the existing diagnostic and optimization to a compile-time determined
value.

This requires silencing a bogus uninit diagnostic in the Fortran
frontend which I've done in a minimal way, avoiding initializing
the 40 byte symbol_attribute structure.  There's several issues,
one is the flag_coarrays is a global variable likely not CSEd
to help the uninit predicate analysis, the other is us short-circuiting
the flag_coarray == GFC_FCOARRAY_LIB && lhs_caf_attr.codimension
accesses as both have no side-effects so the guard isn't effective.
If the frontend folks are happy with this I can localize both
lhs_caf_attr and rhs_caf_attr and copy out the two only flags
tested by the code instead of the somewhat incomplete approach in
the patch.  Any opinions here?

Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

OK for the fortran parts?

Thanks,
Richard.

	PR tree-optimization/99919
	* fold-const.cc (optimize_bit_field_compare): Disable
	transforming the bitfield against constant compare optimization
	if the result is not statically determinable.

gcc/fortran/
	* trans-expr.cc (gfc_trans_assignment_1): Split out
	lhs_codimension from lhs_caf_attr to avoid bogus uninit
	diagnostics.

	* gcc.dg/uninit-pr99919.c: New testcase.
---
 gcc/fold-const.cc                     | 37 +++------------------------
 gcc/fortran/trans-expr.cc             |  6 +++--
 gcc/testsuite/gcc.dg/uninit-pr99919.c | 22 ++++++++++++++++
 3 files changed, 30 insertions(+), 35 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/uninit-pr99919.c

diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
index cdfe3f50ae3..b72cc0a1d51 100644
--- a/gcc/fold-const.cc
+++ b/gcc/fold-const.cc
@@ -4559,7 +4559,6 @@ optimize_bit_field_compare (location_t loc, enum tree_code code,
 {
   poly_int64 plbitpos, plbitsize, rbitpos, rbitsize;
   HOST_WIDE_INT lbitpos, lbitsize, nbitpos, nbitsize;
-  tree type = TREE_TYPE (lhs);
   tree unsigned_type;
   int const_p = TREE_CODE (rhs) == INTEGER_CST;
   machine_mode lmode, rmode;
@@ -4667,13 +4666,7 @@ optimize_bit_field_compare (location_t loc, enum tree_code code,
     }
 
   /* Otherwise, we are handling the constant case.  See if the constant is too
-     big for the field.  Warn and return a tree for 0 (false) if so.  We do
-     this not only for its own sake, but to avoid having to test for this
-     error case below.  If we didn't, we might generate wrong code.
-
-     For unsigned fields, the constant shifted right by the field length should
-     be all zero.  For signed fields, the high-order bits should agree with
-     the sign bit.  */
+     big for the field.  Warn and return a tree for 0 (false) if so.  */
 
   if (lunsignedp)
     {
@@ -4695,31 +4688,9 @@ optimize_bit_field_compare (location_t loc, enum tree_code code,
 	}
     }
 
-  if (nbitpos < 0)
-    return 0;
-
-  /* Single-bit compares should always be against zero.  */
-  if (lbitsize == 1 && ! integer_zerop (rhs))
-    {
-      code = code == EQ_EXPR ? NE_EXPR : EQ_EXPR;
-      rhs = build_int_cst (type, 0);
-    }
-
-  /* Make a new bitfield reference, shift the constant over the
-     appropriate number of bits and mask it with the computed mask
-     (in case this was a signed field).  If we changed it, make a new one.  */
-  lhs = make_bit_field_ref (loc, linner, lhs, unsigned_type,
-			    nbitsize, nbitpos, 1, lreversep);
-
-  rhs = const_binop (BIT_AND_EXPR,
-		     const_binop (LSHIFT_EXPR,
-				  fold_convert_loc (loc, unsigned_type, rhs),
-				  size_int (lbitpos)),
-		     mask);
-
-  lhs = build2_loc (loc, code, compare_type,
-		    build2 (BIT_AND_EXPR, unsigned_type, lhs, mask), rhs);
-  return lhs;
+  /* Otherwise do not prematurely optimize compares of bitfield members
+     to constants.  */
+  return 0;
 }
 \f
 /* Subroutine for fold_truth_andor_1: decode a field reference.
diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
index b95c5cf2f96..12c7dd7f26a 100644
--- a/gcc/fortran/trans-expr.cc
+++ b/gcc/fortran/trans-expr.cc
@@ -11654,9 +11654,11 @@ gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr * expr2, bool init_flag,
 
   /* Only analyze the expressions for coarray properties, when in coarray-lib
      mode.  */
+  bool lhs_codimension = false;
   if (flag_coarray == GFC_FCOARRAY_LIB)
     {
       lhs_caf_attr = gfc_caf_attr (expr1, false, &lhs_refs_comp);
+      lhs_codimension = lhs_caf_attr.codimension;
       rhs_caf_attr = gfc_caf_attr (expr2, false, &rhs_refs_comp);
     }
 
@@ -11738,7 +11740,7 @@ gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr * expr2, bool init_flag,
 
   /* Translate the expression.  */
   rse.want_coarray = flag_coarray == GFC_FCOARRAY_LIB && init_flag
-      && lhs_caf_attr.codimension;
+      && lhs_codimension;
   gfc_conv_expr (&rse, expr2);
 
   /* Deal with the case of a scalar class function assigned to a derived type.  */
@@ -11913,7 +11915,7 @@ gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr * expr2, bool init_flag,
 	}
     }
   else if (flag_coarray == GFC_FCOARRAY_LIB
-	   && lhs_caf_attr.codimension && rhs_caf_attr.codimension
+	   && lhs_codimension && rhs_caf_attr.codimension
 	   && ((lhs_caf_attr.allocatable && lhs_refs_comp)
 	       || (rhs_caf_attr.allocatable && rhs_refs_comp)))
     {
diff --git a/gcc/testsuite/gcc.dg/uninit-pr99919.c b/gcc/testsuite/gcc.dg/uninit-pr99919.c
new file mode 100644
index 00000000000..5ee0e6727b3
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/uninit-pr99919.c
@@ -0,0 +1,22 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -Wuninitialized" } */
+
+struct B { _Bool i: 1; _Bool j: 1; };
+
+_Bool z;
+
+void g (struct B b)
+{
+  _Bool x;
+
+  if (b.i)
+    b.j = 0;
+  else
+    {
+      b.j = b.i;
+      x = b.i;
+    }
+
+  if (b.j)
+    z = x; /* { dg-bogus "uninitialized" } */
+}
-- 
2.35.3

Re: [PATCH] tree-optimization/99919 - bogus uninit diagnostic with bitfield guards

[Richard Biener]

On Thu, Dec 8, 2022 at 12:07 PM Richard Biener via Fortran
<fortran@gcc.gnu.org> wrote:
>
> For the testcase in this PR what fold-const.cc optimize_bit_field_compare
> does to bitfield against constant compares is confusing the uninit
> predicate analysis and it also makes SRA obfuscate instead of optimize
> the code.  We've long had the opinion that those optimizations are
> premature but we do not have any replacement for the more complicated
> ones combining multiple bitfield tests.  The following disables mangling
> the case of a single bitfield test against constants but preserving
> the existing diagnostic and optimization to a compile-time determined
> value.
>
> This requires silencing a bogus uninit diagnostic in the Fortran
> frontend which I've done in a minimal way, avoiding initializing
> the 40 byte symbol_attribute structure.  There's several issues,
> one is the flag_coarrays is a global variable likely not CSEd
> to help the uninit predicate analysis, the other is us short-circuiting
> the flag_coarray == GFC_FCOARRAY_LIB && lhs_caf_attr.codimension
> accesses as both have no side-effects so the guard isn't effective.
> If the frontend folks are happy with this I can localize both
> lhs_caf_attr and rhs_caf_attr and copy out the two only flags
> tested by the code instead of the somewhat incomplete approach in
> the patch.  Any opinions here?
>
> Bootstrapped on x86_64-unknown-linux-gnu, testing in progress.

Testing revealed this regresses

    FAIL: c-c++-common/fold-masked-cmp-1.c  -Wc++-compat
scan-assembler-times testn?b 2
    FAIL: c-c++-common/fold-masked-cmp-2.c  -Wc++-compat
scan-assembler-times testn?b 2

I filed PR108070 for the failure to optimize this on the RTL level
and put this change on hold :/

Richard.

> OK for the fortran parts?
>
> Thanks,
> Richard.
>
>         PR tree-optimization/99919
>         * fold-const.cc (optimize_bit_field_compare): Disable
>         transforming the bitfield against constant compare optimization
>         if the result is not statically determinable.
>
> gcc/fortran/
>         * trans-expr.cc (gfc_trans_assignment_1): Split out
>         lhs_codimension from lhs_caf_attr to avoid bogus uninit
>         diagnostics.
>
>         * gcc.dg/uninit-pr99919.c: New testcase.
> ---
>  gcc/fold-const.cc                     | 37 +++------------------------
>  gcc/fortran/trans-expr.cc             |  6 +++--
>  gcc/testsuite/gcc.dg/uninit-pr99919.c | 22 ++++++++++++++++
>  3 files changed, 30 insertions(+), 35 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.dg/uninit-pr99919.c
>
> diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> index cdfe3f50ae3..b72cc0a1d51 100644
> --- a/gcc/fold-const.cc
> +++ b/gcc/fold-const.cc
> @@ -4559,7 +4559,6 @@ optimize_bit_field_compare (location_t loc, enum tree_code code,
>  {
>    poly_int64 plbitpos, plbitsize, rbitpos, rbitsize;
>    HOST_WIDE_INT lbitpos, lbitsize, nbitpos, nbitsize;
> -  tree type = TREE_TYPE (lhs);
>    tree unsigned_type;
>    int const_p = TREE_CODE (rhs) == INTEGER_CST;
>    machine_mode lmode, rmode;
> @@ -4667,13 +4666,7 @@ optimize_bit_field_compare (location_t loc, enum tree_code code,
>      }
>
>    /* Otherwise, we are handling the constant case.  See if the constant is too
> -     big for the field.  Warn and return a tree for 0 (false) if so.  We do
> -     this not only for its own sake, but to avoid having to test for this
> -     error case below.  If we didn't, we might generate wrong code.
> -
> -     For unsigned fields, the constant shifted right by the field length should
> -     be all zero.  For signed fields, the high-order bits should agree with
> -     the sign bit.  */
> +     big for the field.  Warn and return a tree for 0 (false) if so.  */
>
>    if (lunsignedp)
>      {
> @@ -4695,31 +4688,9 @@ optimize_bit_field_compare (location_t loc, enum tree_code code,
>         }
>      }
>
> -  if (nbitpos < 0)
> -    return 0;
> -
> -  /* Single-bit compares should always be against zero.  */
> -  if (lbitsize == 1 && ! integer_zerop (rhs))
> -    {
> -      code = code == EQ_EXPR ? NE_EXPR : EQ_EXPR;
> -      rhs = build_int_cst (type, 0);
> -    }
> -
> -  /* Make a new bitfield reference, shift the constant over the
> -     appropriate number of bits and mask it with the computed mask
> -     (in case this was a signed field).  If we changed it, make a new one.  */
> -  lhs = make_bit_field_ref (loc, linner, lhs, unsigned_type,
> -                           nbitsize, nbitpos, 1, lreversep);
> -
> -  rhs = const_binop (BIT_AND_EXPR,
> -                    const_binop (LSHIFT_EXPR,
> -                                 fold_convert_loc (loc, unsigned_type, rhs),
> -                                 size_int (lbitpos)),
> -                    mask);
> -
> -  lhs = build2_loc (loc, code, compare_type,
> -                   build2 (BIT_AND_EXPR, unsigned_type, lhs, mask), rhs);
> -  return lhs;
> +  /* Otherwise do not prematurely optimize compares of bitfield members
> +     to constants.  */
> +  return 0;
>  }
>
>  /* Subroutine for fold_truth_andor_1: decode a field reference.
> diff --git a/gcc/fortran/trans-expr.cc b/gcc/fortran/trans-expr.cc
> index b95c5cf2f96..12c7dd7f26a 100644
> --- a/gcc/fortran/trans-expr.cc
> +++ b/gcc/fortran/trans-expr.cc
> @@ -11654,9 +11654,11 @@ gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr * expr2, bool init_flag,
>
>    /* Only analyze the expressions for coarray properties, when in coarray-lib
>       mode.  */
> +  bool lhs_codimension = false;
>    if (flag_coarray == GFC_FCOARRAY_LIB)
>      {
>        lhs_caf_attr = gfc_caf_attr (expr1, false, &lhs_refs_comp);
> +      lhs_codimension = lhs_caf_attr.codimension;
>        rhs_caf_attr = gfc_caf_attr (expr2, false, &rhs_refs_comp);
>      }
>
> @@ -11738,7 +11740,7 @@ gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr * expr2, bool init_flag,
>
>    /* Translate the expression.  */
>    rse.want_coarray = flag_coarray == GFC_FCOARRAY_LIB && init_flag
> -      && lhs_caf_attr.codimension;
> +      && lhs_codimension;
>    gfc_conv_expr (&rse, expr2);
>
>    /* Deal with the case of a scalar class function assigned to a derived type.  */
> @@ -11913,7 +11915,7 @@ gfc_trans_assignment_1 (gfc_expr * expr1, gfc_expr * expr2, bool init_flag,
>         }
>      }
>    else if (flag_coarray == GFC_FCOARRAY_LIB
> -          && lhs_caf_attr.codimension && rhs_caf_attr.codimension
> +          && lhs_codimension && rhs_caf_attr.codimension
>            && ((lhs_caf_attr.allocatable && lhs_refs_comp)
>                || (rhs_caf_attr.allocatable && rhs_refs_comp)))
>      {
> diff --git a/gcc/testsuite/gcc.dg/uninit-pr99919.c b/gcc/testsuite/gcc.dg/uninit-pr99919.c
> new file mode 100644
> index 00000000000..5ee0e6727b3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/uninit-pr99919.c
> @@ -0,0 +1,22 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -Wuninitialized" } */
> +
> +struct B { _Bool i: 1; _Bool j: 1; };
> +
> +_Bool z;
> +
> +void g (struct B b)
> +{
> +  _Bool x;
> +
> +  if (b.i)
> +    b.j = 0;
> +  else
> +    {
> +      b.j = b.i;
> +      x = b.i;
> +    }
> +
> +  if (b.j)
> +    z = x; /* { dg-bogus "uninitialized" } */
> +}
> --
> 2.35.3