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
Martin
Thanks,
Richard.
