[PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code

[PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code

[Richard Biener]

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  <rguenther@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.
---
 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 00000000000..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<vn_constant_hasher> (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
+++ b/gcc/tree-ssa-sccvn.h
@@ -291,6 +291,7 @@ value_id_constant_p (unsigned int v)
 tree fully_constant_vn_reference_p (vn_reference_t);
 tree vn_nary_simplify (vn_nary_op_t);
 
+unsigned do_rpo_vn (function *, edge, bitmap, bool, bool, vn_lookup_kind);
 unsigned do_rpo_vn (function *, edge, bitmap);
 void run_rpo_vn (vn_lookup_kind);
 unsigned eliminate_with_rpo_vn (bitmap);
diff --git a/gcc/tree-ssa-uninit.cc b/gcc/tree-ssa-uninit.cc
index 02e88d58e1f..ab83a4b7978 100644
--- a/gcc/tree-ssa-uninit.cc
+++ b/gcc/tree-ssa-uninit.cc
@@ -38,8 +38,9 @@ along with GCC; see the file COPYING3.  If not see
 #include "builtins.h"
 #include "calls.h"
 #include "gimple-range.h"
-
 #include "gimple-predicate-analysis.h"
+#include "domwalk.h"
+#include "tree-ssa-sccvn.h"
 
 /* This implements the pass that does predicate aware warning on uses of
    possibly uninitialized variables.  The pass first collects the set of
@@ -986,7 +987,19 @@ warn_uninitialized_vars (bool wmaybe_uninit)
   basic_block bb;
   FOR_EACH_BB_FN (bb, cfun)
     {
+      edge_iterator ei;
+      edge e;
+      FOR_EACH_EDGE (e, ei, bb->preds)
+	if (e->flags & EDGE_EXECUTABLE)
+	  break;
+      /* Skip unreachable blocks.  For early analysis we use VN to
+	 determine edge executability when wmaybe_uninit.  */
+      if (!e)
+	continue;
+
       basic_block succ = single_succ (ENTRY_BLOCK_PTR_FOR_FN (cfun));
+      /* ???  This could be improved when we use a greedy walk and have
+	 some edges marked as not executable.  */
       wlims.always_executed = dominated_by_p (CDI_POST_DOMINATORS, succ, bb);
 
       if (wlims.always_executed)
@@ -1319,6 +1332,11 @@ execute_late_warn_uninitialized (function *fun)
 
   calculate_dominance_info (CDI_DOMINATORS);
   calculate_dominance_info (CDI_POST_DOMINATORS);
+
+  /* Mark all edges executable, warn_uninitialized_vars will skip
+     unreachable blocks.  */
+  set_all_edges_as_executable (fun);
+
   /* Re-do the plain uninitialized variable check, as optimization may have
      straightened control flow.  Do this first so that we don't accidentally
      get a "may be" warning when we'd have seen an "is" warning later.  */
@@ -1388,7 +1406,7 @@ make_pass_late_warn_uninitialized (gcc::context *ctxt)
 }
 
 static unsigned int
-execute_early_warn_uninitialized (void)
+execute_early_warn_uninitialized (struct function *fun)
 {
   /* Currently, this pass runs always but
      execute_late_warn_uninitialized only runs with optimization.  With
@@ -1398,6 +1416,17 @@ execute_early_warn_uninitialized (void)
   calculate_dominance_info (CDI_DOMINATORS);
   calculate_dominance_info (CDI_POST_DOMINATORS);
 
+  /* Use VN in its cheapest incarnation and without doing any
+     elimination to compute edge reachability.  Don't bother when
+     we only warn for unconditionally executed code though.  */
+  if (!optimize)
+    {
+      do_rpo_vn (fun, NULL, NULL, false, false, VN_NOWALK);
+      free_rpo_vn ();
+    }
+  else
+    set_all_edges_as_executable (fun);
+
   warn_uninitialized_vars (/*warn_maybe_uninitialized=*/!optimize);
 
   /* Post-dominator information cannot be reliably updated.  Free it
@@ -1412,7 +1441,7 @@ namespace {
 const pass_data pass_data_early_warn_uninitialized =
 {
   GIMPLE_PASS, /* type */
-  "*early_warn_uninitialized", /* name */
+  "early_uninit", /* name */
   OPTGROUP_NONE, /* optinfo_flags */
   TV_TREE_UNINIT, /* tv_id */
   PROP_ssa, /* properties_required */
@@ -1431,9 +1460,9 @@ public:
 
   /* opt_pass methods: */
   virtual bool gate (function *) { return gate_warn_uninitialized (); }
-  virtual unsigned int execute (function *)
+  virtual unsigned int execute (function *fun)
   {
-    return execute_early_warn_uninitialized ();
+    return execute_early_warn_uninitialized (fun);
   }
 
 }; // class pass_early_warn_uninitialized
-- 
2.34.1

Re: [PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code

[Jeff Law]

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  <rguenther@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.

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

Re: [PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code

[Richard Biener]

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  <rguenther@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?

Thanks,
Richard.

Re: [PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code

[Martin Sebor]

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  <rguenther@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.

Re: [PATCH] tree-optimization/104373 - early uninit diagnostic on unreachable code

[Richard Biener]

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  <rguenther@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.

testsuite: Fix up g++.dg/warn/Wuninitialized-32.C test for ilp32 [PR104373]

[Jakub Jelinek]

On Thu, Feb 10, 2022 at 10:57:02AM +0100, Richard Biener via Gcc-patches wrote:
> > >>>   * g++.dg/warn/Wuninitialized-32.C: New testcase.

The testcase FAILs whenever size_t is not unsigned long:
FAIL: g++.dg/warn/Wuninitialized-32.C  -std=c++98 (test for excess errors)
Excess errors:
.../gcc/testsuite/g++.dg/warn/Wuninitialized-32.C:4:7: error: 'operator new' takes type 'size_t' ('unsigned int') as first parameter [-fpermissive]

Fixed by using __SIZE_TYPE__ instead of unsigned long.

Regtested on x86_64-linux -m32/-m64, committed to trunk as obvious.

2022-02-11  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/104373
	* g++.dg/warn/Wuninitialized-32.C (operator new[]): Use __SIZE_TYPE__
	as type of the first argument instead of unsigned long.

--- gcc/testsuite/g++.dg/warn/Wuninitialized-32.C.jj	2022-02-11 00:19:22.376064016 +0100
+++ gcc/testsuite/g++.dg/warn/Wuninitialized-32.C	2022-02-11 00:25:45.194857715 +0100
@@ -1,7 +1,7 @@
 // { dg-do compile }
 // { dg-additional-options "-Wall" }
 
-void* operator new[](unsigned long, void* __p);
+void* operator new[](__SIZE_TYPE__, void* __p);
 
 struct allocator
 {


	Jakub