From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x22c.google.com (mail-lj1-x22c.google.com [IPv6:2a00:1450:4864:20::22c]) by sourceware.org (Postfix) with ESMTPS id 29AA0395C016; Mon, 12 Dec 2022 10:22:14 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 29AA0395C016 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-lj1-x22c.google.com with SMTP id s25so4743851lji.2; Mon, 12 Dec 2022 02:22:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=MWoXy7PCuC0evKgRCI0XnD0U6ORZdlcxda18+IqRADg=; b=n6MinKtt5z0zD6LK9c8X70gOER399XDE51oR+XRBlGL8ALwjL9kmJ9weOt1AfqQCt0 hPJf1iijfrdfjwX7mF1nundBzIprlv+/XjgnqaU/pf9kYmu1dcqWpl8KKbvwsom9SSpz +7MdZ9Gns5BlraYVH2spZdZrOojL/j+V42DhnmkyhWaFOnDHIZfTfCTchpP6QkwxpO8l rwvsJq446jbMN15A1rZTN1AwhjJaaPIT3+EoQH8naOov/A66IMSg7rJQRyRn/1aVqNsO 8js+XBosmMxNI2xZZ+lk/3HKIGB/1lLtbLhG0iPlv1CEo5vqW4sPu7Imfo7ilSAhmnhR Ke6Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=MWoXy7PCuC0evKgRCI0XnD0U6ORZdlcxda18+IqRADg=; b=tr/3Dmw3+cGwdbyrw6xTj+tFinqj0FohMnu+ZPe9Wg/2akqKbQjIWqdeMQUfazvnAz soqhAf0RqUcAa3UkgbjRTPIMh+WAUvXIca5GS2RTVl+mmOOnKnJkiI7vaVGXrMuKQiAy riP562KsCRwShx9H2mWmRXu1GuB6GB/+EfvqsKX1U//0I/nyQfl0j+Y9TplyLYYybaY4 be+bU2NEmkf7MByG1hnYJvlp8SEocoRVhpVs25MhE2Fegk84BoEGmM6SENsTiR8MwFYU HKQo9mrlUFYk7tpjv6hh6AKUHd4xSUz3oxGF34RKX+K9Or2N51J3ehhJC7hq7U0zmj/s leHw== X-Gm-Message-State: ANoB5pnsoL1M0k2YPB+t9e1getv9qU5HYKMHwjrXCMMAwGN2doe6RKTi /VAPlzzG9F74XjUOUblygGx+H4J0owQi7uWPV6I= X-Google-Smtp-Source: AA0mqf406QCTHbKcn8vCIfZ41e96g5WCEqD9Tx5qlrjzHU/bkzk9XXHDE6u3GOXe4CZW+64/629+PRsB26tVNtYg86U= X-Received: by 2002:a05:651c:4d0:b0:27a:b1d:5662 with SMTP id e16-20020a05651c04d000b0027a0b1d5662mr5165401lji.356.1670840532378; Mon, 12 Dec 2022 02:22:12 -0800 (PST) MIME-Version: 1.0 References: In-Reply-To: From: Richard Biener Date: Mon, 12 Dec 2022 11:22:00 +0100 Message-ID: Subject: Re: [PATCH] tree-optimization/99919 - bogus uninit diagnostic with bitfield guards To: Richard Biener Cc: gcc-patches@gcc.gnu.org, fortran@gcc.gnu.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Thu, Dec 8, 2022 at 12:07 PM Richard Biener via Fortran 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