Re: [PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code
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
> >>>
> >>> 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 t
Re: [PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code
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
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
Re: [PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code
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.
Re: [PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code
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. 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. jeff
[PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code
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.
---
gcc/testsuite/g++.dg/warn/Wuninitialized-32.C | 14 +++
gcc/testsuite/gcc.dg/uninit-pr20644-O0.c | 2 +-
gcc/tree-ssa-sccvn.cc | 18 -
gcc/tree-ssa-sccvn.h | 1 +
gcc/tree-ssa-uninit.cc| 39 ---
5 files changed, 58 insertions(+), 16 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/warn/Wuninitialized-32.C
diff --git a/gcc/testsuite/g++.dg/warn/Wuninitialized-32.C
b/gcc/testsuite/g++.dg/warn/Wuninitialized-32.C
new file mode 100644
index 000..8b02b5c6adb
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wuninitialized-32.C
@@ -0,0 +1,14 @@
+// { dg-do compile }
+// { dg-additional-options "-Wall" }
+
+void* operator new[](unsigned long, void* __p);
+
+struct allocator
+{
+ ~allocator();
+};
+
+void *foo (void *p)
+{
+ return p ? new(p) allocator[1] : new allocator[1]; // { dg-bogus
"uninitialized" }
+}
diff --git a/gcc/testsuite/gcc.dg/uninit-pr20644-O0.c
b/gcc/testsuite/gcc.dg/uninit-pr20644-O0.c
index 8ae697abcdf..a335d8cb4eb 100644
--- a/gcc/testsuite/gcc.dg/uninit-pr20644-O0.c
+++ b/gcc/testsuite/gcc.dg/uninit-pr20644-O0.c
@@ -7,7 +7,7 @@ int foo ()
int j;
if (1 == i)
-return j; /* { dg-bogus "uninitialized" "uninitialized" { xfail *-*-* } }
*/
+return j; /* { dg-bogus "uninitialized" "uninitialized" } */
return 0;
}
diff --git a/gcc/tree-ssa-sccvn.cc b/gcc/tree-ssa-sccvn.cc
index a03f0aae924..eb17549c185 100644
--- a/gcc/tree-ssa-sccvn.cc
+++ b/gcc/tree-ssa-sccvn.cc
@@ -7034,15 +7034,14 @@ eliminate_with_rpo_vn (bitmap inserted_exprs)
return walker.eliminate_cleanup ();
}
-static unsigned
+unsigned
do_rpo_vn (function *fn, edge entry, bitmap exit_bbs,
- bool iterate, bool eliminate);
+ bool iterate, bool eliminate, vn_lookup_kind kind);
void
run_rpo_vn (vn_lookup_kind kind)
{
- default_vn_walk_kind = kind;
- do_rpo_vn (cfun, NULL, NULL, true, false);
+ do_rpo_vn (cfun, NULL, NULL, true, false, kind);
/* ??? Prune requirement of these. */
constant_to_value_id = new hash_table (23);
@@ -7740,11 +7739,12 @@ do_unwind (unwind_state *to, rpo_elim &avail)
executed and iterate. If ELIMINATE is true then perform
elimination, otherwise leave that to the caller. */
-static unsigned
+unsigned
do_rpo_vn (function *fn, edge entry, bitmap exit_bbs,
- bool iterate, bool eliminate)
+ bool iterate, bool eliminate, vn_lookup_kind kind)
{
unsigned todo = 0;
+ default_vn_walk_kind = kind;
/* We currently do not support region-based iteration when
elimination is requested. */
@@ -8164,8 +8164,7 @@ do_rpo_vn (function *fn, edge entry, bitmap exit_bbs,
unsigned
do_rpo_vn (function *fn, edge entry, bitmap exit_bbs)
{
- default_vn_walk_kind = VN_WALKREWRITE;
- unsigned todo = do_rpo_vn (fn, entry, exit_bbs, false, true);
+ unsigned todo = do_rpo_vn (fn, entry, exit_bbs, false, true, VN_WALKREWRITE);
free_rpo_vn ();
return todo;
}
@@ -8221,8 +8220,7 @@ pass_fre::execute (function *fun)
if (iterate_p)
loop_optimizer_init (AVOID_CFG_MODIFICATIONS);
- default_vn_walk_kind = VN_WALKREWRITE;
- todo = do_rpo_vn (fun, NULL, NULL, iterate_p, true);
+ todo = do_rpo_vn (fun, NULL, NULL, iterate_p, true, VN_WALKREWRITE);
free_rpo_vn ();
if (iterate_p)
diff --git a/gcc/tree-ssa-sccvn.h b/gcc/tree-ssa-sccvn.h
index f161b80417b..aeed07039b7 100644
--- a/gcc/tree-ssa-sccvn.h
++
