[PATCH][RFC] Uninit diagnostic consistency

[PATCH][RFC] Uninit diagnostic consistency

[Richard Biener]

I've figured that while the uninit diagnostic is intended to diagnose
all cases where we cannot prove the must-uninit paths are not reaching
any use the actual implementation errors on the wrong side in two
places.  First the recently added use_cannot_happen computes the
must-uninit predicate using compute_control_dep_chain which errors
on the side making the covered domain too small (pruning OR components),
second, init_use_preds which computes the predicate that holds when
the use is executed also uses the same function.  In both cases the
computational limits implemented in compute_control_dep_chain will cause
false negatives.  The following corrects this by using the recently
introduced simple_control_dep_chain instead which errors on the side
of making the domain larger (pruning AND components).

I shall note that I didn't yet find a testcase that requires
use_cannot_happen which does the same as the init_from_phi_def
and superset_of combination.  The only difference is that
init_from_phi_def computes conjunction of the edge predicates
covering the not must-uninitialized control flow, also handling
the case of PHI cascading and verifying the use predicate is
a subset of this while use_cannot_happen computes whether
each predicate on the uninitialized edges (may-uninit for the
case of PHI cascading) has an empty intersection with the
use predicate.  Due to the PHI cascading handling the original
should be superior but since both implementations apply the
computational limit of compute_control_dep_chain in different
ways and have different false negative issues it may have catched
some extra bits at times.  A patch to remove use_cannot_happen
passes bootstrap and regtest with all languages on x86_64, my
preference would be to axe it instead of fixing it.

Note I am yet not sure why compute_control_dep_chain goes the
length of computing all (well, only some) paths from the
root to the PHI edge source rather than walking dominators
as simple_control_dep_chain or using control dependences (and
pruning them appropriately).

The patch as-is causes

FAIL: g++.dg/warn/uninit-pr55288.C (test for bogus messages, line 16)

the testcase was fixed by r0-125162-g5c2961cf38a69f, the issue here
is that simple_control_dep_chain does not handle A || A with the
same predicate on different paths.

XPASS: gcc.dg/uninit-pred-7_a.c bogus warning (test for bogus messages, line 26)
FAIL: gcc.dg/uninit-pred-9_b.c pr101674 (test for bogus messages, line 23)

where the FAIL is PR101674.  I've commented extensively in the PR.

Bootstrapped on x86_64-unknown-linux-gnu, full testing is still in
progress.

	* gimple-predicate-analysis.cc (dump_dep_chains): Don't
	use auto_vec[] arguments.
	(uninit_analysis::use_cannot_happen): Only use
	simple_control_dep_chain.
	(uninit_analysis::init_use_preds): Likewise.
	(uninit_analysis::init_from_phi_def): Add comment why
	using compute_control_dep_chain here is OK.
---
 gcc/gimple-predicate-analysis.cc | 72 ++++++++++++--------------------
 1 file changed, 27 insertions(+), 45 deletions(-)

diff --git a/gcc/gimple-predicate-analysis.cc b/gcc/gimple-predicate-analysis.cc
index 079e06009fd..44f00507d83 100644
--- a/gcc/gimple-predicate-analysis.cc
+++ b/gcc/gimple-predicate-analysis.cc
@@ -238,14 +238,14 @@ dump_predicates (gimple *stmt, const pred_chain_union &preds, const char *msg)
 /* Dump the first NCHAINS elements of the DEP_CHAINS array into DUMP_FILE.  */
 
 static void
-dump_dep_chains (const auto_vec<edge> dep_chains[], unsigned nchains)
+dump_dep_chains (const vec<edge> dep_chains[], unsigned nchains)
 {
   if (!dump_file)
     return;
 
   for (unsigned i = 0; i != nchains; ++i)
     {
-      const auto_vec<edge> &v = dep_chains[i];
+      const vec<edge> &v = dep_chains[i];
       unsigned n = v.length ();
       for (unsigned j = 0; j != n; ++j)
 	{
@@ -1309,41 +1309,27 @@ uninit_analysis::use_cannot_happen (gphi *phi, unsigned opnds, const predicate &
 	continue;
 
       edge e = gimple_phi_arg_edge (phi, i);
-      auto_vec<edge> dep_chains[MAX_NUM_CHAINS];
-      auto_vec<edge, MAX_CHAIN_LEN + 1> cur_chain;
-      unsigned num_chains = 0;
-      unsigned num_calls = 0;
 
-      /* Build the control dependency chain for the PHI argument...  */
-      if (!compute_control_dep_chain (cd_root,
-				      e->src, dep_chains, &num_chains,
-				      cur_chain, &num_calls))
-	{
-	  gcc_assert (num_chains == 0);
-	  /* If compute_control_dep_chain bailed out due to limits
-	     build a partial sparse path using dominators.  Collect
-	     only edges whose predicates are always true when reaching E.  */
-	  simple_control_dep_chain (dep_chains[0], cd_root, e);
-	  num_chains++;
-	}
-      /* Update the chains with the phi operand edge.  */
-      else if (EDGE_COUNT (e->src->succs) > 1)
-	{
-	  for (unsigned j = 0; j < num_chains; j++)
-	    dep_chains[j].safe_push (e);
-	}
+      /* We cannot use compute_control_dep_chain here because that does
+	 not give us a conservative answer for proving must initialized
+	 since the domain is equal or less when it runs into its
+	 computational limits.  simple_control_dep_chain errors on
+	 the other side so we can use that here to compute the
+	 must-uninitialized predicate.  */
+      auto_vec<edge, MAX_CHAIN_LEN + 1> cur_chain;
+      simple_control_dep_chain (cur_chain, cd_root, e);
 
       if (DEBUG_PREDICATE_ANALYZER && dump_file)
 	{
 	  fprintf (dump_file, "predicate::use_cannot_happen (...) "
 		   "dep_chains for arg %u:\n\t", i);
-	  dump_dep_chains (dep_chains, num_chains);
+	  dump_dep_chains (&cur_chain, 1);
 	}
 
       /* ...and convert it into a set of predicates guarding its
 	 definition.  */
       predicate def_preds;
-      def_preds.init_from_control_deps (dep_chains, num_chains);
+      def_preds.init_from_control_deps (&cur_chain, 1);
       if (def_preds.is_empty ())
 	/* If there's no predicate there's no basis to rule the use out.  */
 	return false;
@@ -2147,36 +2133,27 @@ uninit_analysis::init_use_preds (predicate &use_preds, basic_block def_bb,
       break;
     }
 
-  /* Set DEP_CHAINS to the set of edges between CD_ROOT and USE_BB.
-     Each DEP_CHAINS element is a series of edges whose conditions
-     are logical conjunctions.  Together, the DEP_CHAINS vector is
-     used below to initialize an OR expression of the conjunctions.  */
-  unsigned num_calls = 0;
-  unsigned num_chains = 0;
-  auto_vec<edge> dep_chains[MAX_NUM_CHAINS];
+  /* compute_control_dep_chain, when it runs into computational
+     limits, errors on the side of making the domain smaller which
+     is not OK for the use predicate computation since our desire is
+     to diagnose uses that cannot be proved must-initialized.
+     Use simple_control_dep_chain instead.  */
   auto_vec<edge, MAX_CHAIN_LEN + 1> cur_chain;
-
-  if (!compute_control_dep_chain (cd_root, use_bb, dep_chains, &num_chains,
-				  cur_chain, &num_calls))
-    {
-      gcc_assert (num_chains == 0);
-      simple_control_dep_chain (dep_chains[0], cd_root, use_bb);
-      num_chains++;
-    }
+  simple_control_dep_chain (cur_chain, cd_root, use_bb);
 
   if (DEBUG_PREDICATE_ANALYZER && dump_file)
     {
       fprintf (dump_file, "predicate::predicate (def_bb = %u, use_bb = %u, func_t) "
-	       "initialized from %u dep_chains:\n\t",
-	       def_bb->index, use_bb->index, num_chains);
-      dump_dep_chains (dep_chains, num_chains);
+	       "initialized from dep_chain:\n\t",
+	       def_bb->index, use_bb->index);
+      dump_dep_chains (&cur_chain, 1);
     }
 
   /* From the set of edges computed above initialize *THIS as the OR
      condition under which the definition in DEF_BB is used in USE_BB.
      Each OR subexpression is represented by one element of DEP_CHAINS,
      where each element consists of a series of AND subexpressions.  */
-  use_preds.init_from_control_deps (dep_chains, num_chains);
+  use_preds.init_from_control_deps (&cur_chain, 1);
   return !use_preds.is_empty ();
 }
 
@@ -2246,6 +2223,11 @@ uninit_analysis::init_from_phi_def (gphi *phi)
       edge e = def_edges[i];
       unsigned num_calls = 0;
       unsigned prev_nc = num_chains;
+      /* This computes the domain for must-initialized of the PHI
+	 def based on the edge predicates.  That errors on the
+	 conservative side, making the must-initialized domain
+	 smaller, when failing to consider paths if compute_control_dep_chain
+	 runs into its computational limits.  */
       compute_control_dep_chain (cd_root, e->src, dep_chains,
 				 &num_chains, cur_chain, &num_calls);
 
-- 
2.35.3