On Wed, 9 Feb 2022, Martin Sebor wrote:
> On 2/9/22 00:12, Richard Biener wrote:
> > 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 <rguent...@suse.de>
> >>>
> >>> 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?
>
> Many middle end warnings now run at -O0. Thanks to Ranger (and
> the pointer_query solution), they can identify many of the same
> problem statements as with optimization. But for the same reason
> they're also more prone to false positives for unreachable code
> because DCE doesn't run at -O0. So in my mind, identifying at
> least some of it then, is a step in the right direction.
>
> So for the avoidance of doubt, I'm in favor of both the patch and
> extending the approach to other warnings.
Thanks, I'm going to push the patch then.
Richard.
