Re: [PATCH] PR91195: fix -Wmaybe-uninitialized warning for conditional store optimization

[Jakub Jelinek <jakub@redhat.com>]

On Tue, Sep 03, 2019 at 02:27:29PM -0600, Jeff Law wrote:
> >> +      /* The transformation below will inherently introduce a memory load,
> >> +	 for which LHS may not be initialized yet if it is not in NOTRAP,
> >> +	 so a -Wmaybe-uninitialized warning message could be triggered.
> >> +	 Since it's a bit hard to have a very accurate uninitialization
> >> +	 analysis to memory reference, we disable the warning here to avoid
> >> +	 confusion.  */
> >> +      TREE_NO_WARNING (lhs) = 1;
> > 
> > I don't like this, but not for the reasons Martin stated, we use
> > TREE_NO_WARNING not just when we've emitted warnings, but in many places
> > when we've done something that might trigger false positives.
> > Yes, it would be nice to do it more selectively.
> > 
> > The problem I see with the above though is that lhs might very well be
> > a decl, and setting TREE_NO_WARNING on it then doesn't affect only the
> > hoisted load, but also all other code that refers to the decl.
> LHS is restricted to just MEM_REF, ARRAY_REF and COMPONENT_REF.  We'd be
> setting the NO_WARNING bits on the toplevel expression, but not on
> anything shared like a _DECL node.
> 
> So what we're losing here would be things like out of bounds array
> checks on the LHS, so it still sucks.

Sorry for dropping the ball on this.
You're right, LHS is a MEM_REF, ARRAY_REF or COMPONENT_REF, so what I was
worried about doesn't happen.
I've tried using gimple_set_no_warning, but tree-ssa-uninit.c in this case
actually doesn't look at that (it does only in a different spot).

> > If the TREE_NO_WARNING bit is set on something that isn't shareable, that is
> > fine with me, like a MEM_REF, TARGET_MEM_REF or handled component.  If lhs
> > is a decl, can we force a MEM_REF around it (and won't we fold it back to
> > the decl?)?  Or perhaps better, can we gimple_set_no_warning on the load
> > stmt instead?
> We have the toplevel statement, so that might be worth a try as well.

But, what the patch did was set it on the tree that is later unshared,
which means TREE_NO_WARNING wasn't set just on the rhs1 of the load, but
also on the lhs of the store.

The following version fixes that and I've also added the testcase to the
testsuite.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2019-11-19  Jiangning Liu  <jiangning.liu@amperecomputing.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/91195
	* tree-ssa-phiopt.c (cond_store_replacement): Move lhs unsharing
	earlier.  Set TREE_NO_WARNING on the rhs1 of the artificially added
	load.

	* gcc.dg/pr91195.c: New test.

--- gcc/tree-ssa-phiopt.c.jj	2019-11-13 10:54:53.882917365 +0100
+++ gcc/tree-ssa-phiopt.c	2019-11-19 20:51:33.345775363 +0100
@@ -2269,6 +2269,10 @@ cond_store_replacement (basic_block midd
   name = make_temp_ssa_name (TREE_TYPE (lhs), NULL, "cstore");
   new_stmt = gimple_build_assign (name, lhs);
   gimple_set_location (new_stmt, locus);
+  lhs = unshare_expr (lhs);
+  /* Set TREE_NO_WARNING on the rhs of the load to avoid uninit
+     warnings.  */
+  TREE_NO_WARNING (gimple_assign_rhs1 (new_stmt)) = 1;
   gsi_insert_on_edge (e1, new_stmt);
 
   /* 3) Create a PHI node at the join block, with one argument
@@ -2279,7 +2283,6 @@ cond_store_replacement (basic_block midd
   add_phi_arg (newphi, rhs, e0, locus);
   add_phi_arg (newphi, name, e1, locus);
 
-  lhs = unshare_expr (lhs);
   new_stmt = gimple_build_assign (lhs, PHI_RESULT (newphi));
 
   /* 4) Insert that PHI node.  */
--- gcc/testsuite/gcc.dg/pr91195.c.jj	2019-11-19 20:57:57.841024685 +0100
+++ gcc/testsuite/gcc.dg/pr91195.c	2019-11-19 20:57:40.767280052 +0100
@@ -0,0 +1,25 @@
+/* PR middle-end/91195 */
+/* { dg-do compile } */
+/* { dg-options "-Wmaybe-uninitialized -O2" } */
+
+int bar (char*);
+
+void
+foo (char *x, char *y)
+{
+  char *a[2];
+  int b = 0;
+
+  if (x)
+    a[b++] = x;		/* { dg-bogus "may be used uninitialized in this function" } */
+  if (y)
+    a[b++] = y;
+
+  for (int j = 0; j < 4; j++) 
+    switch (j)
+      {
+      case 0:
+	if (b == 0 || bar (a[0]))
+	  break;
+      }
+}


	Jakub