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

[Richard Biener <richard.guenther@gmail.com>]

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