On Thu, Dec 8, 2022 at 12:07 PM Richard Biener via Fortran
<[email protected]> 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
