From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 06B773858D20 for ; Wed, 9 Feb 2022 07:12:40 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 06B773858D20 Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id C1B1A1F391; Wed, 9 Feb 2022 07:12:38 +0000 (UTC) Received: from murzim.suse.de (murzim.suse.de [10.160.4.192]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id B73D8A3B83; Wed, 9 Feb 2022 07:12:38 +0000 (UTC) Date: Wed, 9 Feb 2022 08:12:38 +0100 (CET) From: Richard Biener To: Jeff Law cc: gcc-patches@gcc.gnu.org, msebor@redhat.com Subject: Re: [PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code In-Reply-To: Message-ID: References: <20220208070313.8E16013483@imap2.suse-dmz.suse.de> MIME-Version: 1.0 X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Feb 2022 07:12:41 -0000 On Tue, 8 Feb 2022, Jeff Law wrote: > > > On 2/8/2022 12:03 AM, Richard Biener via Gcc-patches wrote: > > The following improves early uninit diagnostics by computing edge > > reachability using our value-numbering framework in its cheapest > > mode and ignoring unreachable blocks when looking > > for uninitialized uses. To not ICE with -fdump-tree-all the > > early uninit pass needs a dumpfile since VN tries to dump statistics. > > > > For gimple-match.c at -O0 -g this causes a 2% increase in compile-time. > > > > In theory all early diagnostic passes could benefit from a VN run but > > that would require more refactoring that's not appropriate at this stage. > > This patch addresses a GCC 12 diagnostic regression and also happens to > > fix one XFAIL in gcc.dg/uninit-pr20644-O0.c > > > > Bootstrapped and tested on x86_64-unknown-linux-gnu, OK for trunk? > > > > Thanks, > > Richard. > > > > 2022-02-04 Richard Biener > > > > PR tree-optimization/104373 > > * tree-ssa-sccvn.h (do_rpo_vn): New export exposing the > > walk kind. > > * tree-ssa-sccvn.cc (do_rpo_vn): Export, get the default > > walk kind as argument. > > (run_rpo_vn): Adjust. > > (pass_fre::execute): Likewise. > > * tree-ssa-uninit.cc (warn_uninitialized_vars): Skip > > blocks not reachable. > > (execute_late_warn_uninitialized): Mark all edges as > > executable. > > (execute_early_warn_uninitialized): Use VN to compute > > executable edges. > > (pass_data_early_warn_uninitialized): Enable a dump file, > > change dump name to warn_uninit. > > > > * g++.dg/warn/Wuninitialized-32.C: New testcase. > > * gcc.dg/uninit-pr20644-O0.c: Remove XFAIL. > I'm conflicted on this ;-) > > I generally lean on the side of eliminating false positives in these > diagnostics.  So identifying unreachable blocks and using that to prune the > set of warnings we generate, even at -O0 is good from that point of view. > > But doing something like this has many of the problems that relying on > optimizations does, even if we don't optimize away the unreachable code.  > Right now the warning should be fairly stable at -O0 -- the set of diagnostics > you get isn't going to change a lot release to release which is important to > some users.   Second, at -O0 whether or not you get a warning isn't a function > of how good our unreachable code analysis might be. > > This was quite a contentious topic many years ago.  So much that I dropped > some work on Wuninit on the floor as I just couldn't take the arguing.  So be > aware that you might be opening a can of worms. > > So the question comes down to a design decision.   What's more important to > the end users?  Fewer false positives or better stability in the warning?  I > think the former, but there's certainly been a vocal group that prefers the > latter. I see - I didn't think of this aspect at all but that means I have no idea on whether it is important or not ... In our own setup we're running into "instabilities" with optimization when building packages that enable -Werror, so I can see shops doing dev builds at -O0 with warnings and -Werror but drop -Werror for optimized builds. > On the implementation side I have zero concerns.    Looking further out, ISTM > we could mark the blocks as unreachable (rather than deducing it from edge > flags).  That would make it pretty easy to mark those blocks relatively early > and allow us to suppress any middle end diagnostic occurring in an unreachable > block. So what I had in mind is that for the set of early diagnostic passes PUSH_INSERT_PASSES_WITHIN (pass_build_ssa_passes) NEXT_PASS (pass_fixup_cfg); NEXT_PASS (pass_build_ssa); NEXT_PASS (pass_warn_printf); NEXT_PASS (pass_warn_nonnull_compare); NEXT_PASS (pass_early_warn_uninitialized); NEXT_PASS (pass_warn_access, /*early=*/true); we'd run VN and keep it's lattice around (and not just the EDGE_EXECUTABLE flags). That would for example allow pass_warn_nonnull_compare to see that in void foo (void *p __attribute__((nonnull))) { void *q = p; if (q) bar (q); } we are comparing a never NULL pointer. Currently the q = p copy makes it not realize this. Likewise some constants can be propagated this way. Of course using the VN lattice means quite some changes in those passes. Even without the VN lattice having unreachable edges marked could improve diagnostics for, say PHI nodes, if only a single executable edge remains. Martin, do you have any thoughts here? Any opinion on the patch that for now just marks (not) executable edges to prune diagnostics at -O0? Thanks, Richard.