match.pd, expand: Fold VCE from integer with [0, 1] range to bool into NOP_EXPR [PR80635]

Hi!

SRA creates a VCE from integer to bool and that VCE then prevents other
optimizations or e.g. prevents the uninit pass from avoiding a false
positive warning.

In the PR using NOP_EXPR has been discussed as one possibility and has been
rejected because at expansion it will emit a superfluous & 1 operation.
I still think it is a good idea to use NOP_EXPR and so have changed
expansion to not emit that & 1 operation in that case.  Both spots are
done with tight conditions (bool only etc.), as I'd like to fix just
that case and not introduce a wider general optimization, but perhaps
later we could lift it and do a general range of arbitrary
type_has_mode_precision_p to non-type_has_mode_precision_p with same
TYPE_MODE case.

On this particular case, the expr.c part isn't really needed, because we
have an unsigned char SSA_NAME with values 0 or 1, VCE to bool and
comparison of that bool against false in a condition, and with the
match.pd change the VCE is simplified into NOP_EXPR and that is later
folded into just comparison of the unsigned char value against 0.
If I under the debugger without the match.pd change the VCE into NOP_EXPR
at the start of TER, then without the expr.c change indeed & 1 is emitted
but then combine removes it again (non-zero bits logic makes that possible),
with the expr.c change we don't emit it at all, which is good because
we can't rely on the combine managing to do it in all cases.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2021-02-24  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/80635
	* match.pd (view_convert<bool> (A) -> (bool) (A)): Simplify VCE of
	ssa_name_has_boolean_range with integral type to bool.
	* expr.c (expand_expr_real_2): Avoid REDUCE_BIT_FIELD for cast to
	boolean if operand is ssa_name_has_boolean_range.

	* g++.dg/warn/pr80635-1.C: New test.
	* g++.dg/warn/pr80635-2.C: New test.


	Jakub

On Wed, 24 Feb 2021, Jakub Jelinek wrote:

> Hi!
> 
> SRA creates a VCE from integer to bool and that VCE then prevents other
> optimizations or e.g. prevents the uninit pass from avoiding a false
> positive warning.
> 
> In the PR using NOP_EXPR has been discussed as one possibility and has been
> rejected because at expansion it will emit a superfluous & 1 operation.
> I still think it is a good idea to use NOP_EXPR and so have changed
> expansion to not emit that & 1 operation in that case.  Both spots are
> done with tight conditions (bool only etc.), as I'd like to fix just
> that case and not introduce a wider general optimization, but perhaps
> later we could lift it and do a general range of arbitrary
> type_has_mode_precision_p to non-type_has_mode_precision_p with same
> TYPE_MODE case.

But it still is a pessimization.  VCE says there's no code to be
generated but NOP_EXPR says there is a conversion involved, even
if you later elide it via ssa_name_has_boolean_range.

So I wonder what other optimizations are prevented here?

Why does uninit warn with VCE but not with NOP_EXPR?  Or does the
warning disappear because of those other optimizations you mention?


> On this particular case, the expr.c part isn't really needed, because we
> have an unsigned char SSA_NAME with values 0 or 1, VCE to bool and
> comparison of that bool against false in a condition, and with the
> match.pd change the VCE is simplified into NOP_EXPR and that is later
> folded into just comparison of the unsigned char value against 0.
> If I under the debugger without the match.pd change the VCE into NOP_EXPR
> at the start of TER, then without the expr.c change indeed & 1 is emitted
> but then combine removes it again (non-zero bits logic makes that possible),
> with the expr.c change we don't emit it at all, which is good because
> we can't rely on the combine managing to do it in all cases.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2021-02-24  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/80635
> 	* match.pd (view_convert<bool> (A) -> (bool) (A)): Simplify VCE of
> 	ssa_name_has_boolean_range with integral type to bool.
> 	* expr.c (expand_expr_real_2): Avoid REDUCE_BIT_FIELD for cast to
> 	boolean if operand is ssa_name_has_boolean_range.
> 
> 	* g++.dg/warn/pr80635-1.C: New test.
> 	* g++.dg/warn/pr80635-2.C: New test.
> 
> --- gcc/match.pd.jj	2021-02-18 16:21:01.000000000 +0100
> +++ gcc/match.pd	2021-02-23 13:16:15.095416289 +0100
> @@ -3316,7 +3316,13 @@ (define_operator_list COND_TERNARY
>    (view_convert @0)
>    (if ((INTEGRAL_TYPE_P (type) || POINTER_TYPE_P (type))
>         && (INTEGRAL_TYPE_P (TREE_TYPE (@0)) || POINTER_TYPE_P (TREE_TYPE (@0)))
> -       && TYPE_PRECISION (type) == TYPE_PRECISION (TREE_TYPE (@0)))
> +       && (TYPE_PRECISION (type) == TYPE_PRECISION (TREE_TYPE (@0))
> +	   || (TREE_CODE (type) == BOOLEAN_TYPE
> +	       && TREE_CODE (@0) == SSA_NAME
> +	       && TREE_CODE (TREE_TYPE (@0)) != BOOLEAN_TYPE
> +	       && TYPE_MODE (type) == TYPE_MODE (TREE_TYPE (@0))
> +	       && type_has_mode_precision_p (TREE_TYPE (@0))
> +	       && ssa_name_has_boolean_range (@0))))
>     (convert @0)))
>  
>  /* Strip inner integral conversions that do not change precision or size, or
> --- gcc/expr.c.jj	2021-02-02 10:01:26.483903572 +0100
> +++ gcc/expr.c	2021-02-23 13:10:26.400323532 +0100
> @@ -8790,6 +8790,15 @@ expand_expr_real_2 (sepops ops, rtx targ
>  	      && GET_CODE (op0) == SUBREG)
>  	    SUBREG_PROMOTED_VAR_P (op0) = 0;
>  
> +	  /* Don't reduce to boolean range if we know the operand
> +	     already has a boolean range.  */
> +	  if (reduce_bit_field
> +	      && TREE_CODE (type) == BOOLEAN_TYPE
> +	      && TREE_CODE (treeop0) == SSA_NAME
> +	      && TREE_CODE (TREE_TYPE (treeop0)) != BOOLEAN_TYPE
> +	      && type_has_mode_precision_p (TREE_TYPE (treeop0))
> +	      && ssa_name_has_boolean_range (treeop0))
> +	    return op0;
>  	  return REDUCE_BIT_FIELD (op0);
>  	}
>  
> --- gcc/testsuite/g++.dg/warn/pr80635-1.C.jj	2021-02-23 13:17:35.398516088 +0100
> +++ gcc/testsuite/g++.dg/warn/pr80635-1.C	2021-02-23 13:19:21.712324315 +0100
> @@ -0,0 +1,46 @@
> +// PR tree-optimization/80635
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-O2 -Wmaybe-uninitialized" }
> +
> +using size_t = decltype (sizeof (1));
> +inline void *operator new (size_t, void *p) { return p; }
> +template<typename T>
> +struct optional
> +{
> +  optional () : m_dummy (), live (false) {}
> +  void emplace () { new (&m_item) T (); live = true; }
> +  ~optional () { if (live) m_item.~T (); }
> +
> +  union
> +  {
> +    struct {} m_dummy;
> +    T m_item;
> +  };
> +  bool live;
> +};
> +
> +extern int get ();
> +extern void set (int);
> +
> +struct A
> +{
> +  A () : m (get ()) {}
> +  ~A () { set (m); }	// { dg-bogus "may be used uninitialized in this function" }
> +
> +  int m;
> +};
> +
> +struct B
> +{
> +  B ();
> +  ~B ();
> +};
> +
> +void func ()
> +{
> +  optional<A> maybe_a;
> +  optional<B> maybe_b;
> +
> +  maybe_a.emplace ();
> +  maybe_b.emplace ();
> +}
> --- gcc/testsuite/g++.dg/warn/pr80635-2.C.jj	2021-02-23 13:17:38.426482145 +0100
> +++ gcc/testsuite/g++.dg/warn/pr80635-2.C	2021-02-23 13:27:34.215803360 +0100
> @@ -0,0 +1,31 @@
> +// PR tree-optimization/80635
> +// { dg-do compile { target c++17 } }
> +// { dg-options "-O2 -Wmaybe-uninitialized" }
> +
> +#include <optional>
> +
> +extern int get ();
> +extern void set (int);
> +
> +struct A
> +{
> +  A () : m (get ()) {}
> +  ~A () { set (m); }	// { dg-bogus "may be used uninitialized in this function" }
> +
> +  int m;
> +};
> +
> +struct B
> +{
> +  B ();
> +  ~B ();
> +};
> +
> +void func ()
> +{
> +  std::optional<A> maybe_a;
> +  std::optional<B> maybe_b;
> +
> +  maybe_a.emplace ();
> +  maybe_b.emplace ();
> +}
> 
> 	Jakub
> 
>

On Wed, Feb 24, 2021 at 11:50:10AM +0100, Richard Biener wrote:
> > In the PR using NOP_EXPR has been discussed as one possibility and has been
> > rejected because at expansion it will emit a superfluous & 1 operation.
> > I still think it is a good idea to use NOP_EXPR and so have changed
> > expansion to not emit that & 1 operation in that case.  Both spots are
> > done with tight conditions (bool only etc.), as I'd like to fix just
> > that case and not introduce a wider general optimization, but perhaps
> > later we could lift it and do a general range of arbitrary
> > type_has_mode_precision_p to non-type_has_mode_precision_p with same
> > TYPE_MODE case.
> 
> But it still is a pessimization.  VCE says there's no code to be
> generated but NOP_EXPR says there is a conversion involved, even
> if you later elide it via ssa_name_has_boolean_range.

I'm not convinced it is a pessimization.
Because, a NOP_EXPR is something the compiler can optimize orders of
magnitude more than VCE.
To back that up by some random numbers,
grep CASE_CONVERT: gimple-match.c | wc -l; grep VIEW_CONVERT_EXPR: gimple-match.c | wc -l
417
18

> So I wonder what other optimizations are prevented here?

> Why does uninit warn with VCE but not with NOP_EXPR?  Or does the
> warning disappear because of those other optimizations you mention?

The optimization that it prevents is in this particular case in tree-vrp.c
(vrp_simplify_cond_using_ranges):

      if (!is_gimple_assign (def_stmt)
          || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def_stmt)))
        return;
so it punts on VIEW_CONVERT_EXPR, with NOP_EXPR it optimizes that:
  _9 = (bool) maybe_a$4_7;
  if (_9 != 0)
into:
  _9 = (bool) maybe_a$4_7;
  if (maybe_a$4_7 != 0)

Now, if I apply my patch but manually disable this
vrp_simplify_cond_using_ranges optimization, then the uninit warning is
back, so on the uninit side it is not about VIEW_CONVERT_EXPR vs. NOP_EXPR,
both are bad there, uninit wants the guarding condition to be
that SSA_NAME and not some demotion cast thereof.
We have:
  # maybe_a$m_6 = PHI <_5(4), maybe_a$m_4(D)(6)>
  # maybe_a$4_7 = PHI <1(4), 0(6)>
...
One of:
  _9 = VIEW_CONVERT_EXPR<bool>(maybe_a$4_7);
  if (_9 != 0)
or:
  _9 = (bool) maybe_a$4_7;
  if (_9 != 0)
or:
  if (maybe_a$4_7 != 0)
followed by:
    goto <bb 11>; [0.00%]
  else
    goto <bb 14>; [0.00%]
...
  <bb 11> [count: 0]:
  set (maybe_a$m_6);
and uninit wants to see that maybe_a$m_4(D) is not used if
bb 11 is encountered.

So, if you are strongly opposed to the posted patch, I guess the fix can be
(at least fixes the testcase; completely untested except for
make check-c++-all RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} dg.exp=pr80635*.C'
) following.
But, I fear there will be dozens of other spots where we'll punt on
optimizing when it is a VCE rather than NOP_EXPR.

2021-02-24  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/80635
	* tree-vrp.c (vrp_simplify_cond_using_ranges): Also handle
	VIEW_CONVERT_EXPR if modes are the same, innerop is integral and
	has mode precision.

	* g++.dg/warn/pr80635-1.C: New test.
	* g++.dg/warn/pr80635-2.C: New test.

--- gcc/tree-vrp.c.jj	2021-02-24 12:56:58.573939572 +0100
+++ gcc/tree-vrp.c	2021-02-24 13:05:22.675326780 +0100
@@ -4390,11 +4390,24 @@ vrp_simplify_cond_using_ranges (vr_value
       gimple *def_stmt = SSA_NAME_DEF_STMT (op0);
       tree innerop;
 
-      if (!is_gimple_assign (def_stmt)
-	  || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def_stmt)))
+      if (!is_gimple_assign (def_stmt))
 	return;
 
-      innerop = gimple_assign_rhs1 (def_stmt);
+      switch (gimple_assign_rhs_code (def_stmt))
+	{
+	CASE_CONVERT:
+	  innerop = gimple_assign_rhs1 (def_stmt);
+	  break;
+	case VIEW_CONVERT_EXPR:
+	  innerop = TREE_OPERAND (gimple_assign_rhs1 (def_stmt), 0);
+	  if (TYPE_MODE (TREE_TYPE (op0)) != TYPE_MODE (TREE_TYPE (innerop))
+	      || !INTEGRAL_TYPE_P (TREE_TYPE (innerop))
+	      || !type_has_mode_precision_p (TREE_TYPE (innerop)))
+	    return;
+	  break;
+	default:
+	  break;
+	}
 
       if (TREE_CODE (innerop) == SSA_NAME
 	  && !POINTER_TYPE_P (TREE_TYPE (innerop))
--- gcc/testsuite/g++.dg/warn/pr80635-1.C.jj	2021-02-24 12:24:15.176834532 +0100
+++ gcc/testsuite/g++.dg/warn/pr80635-1.C	2021-02-24 12:24:15.176834532 +0100
@@ -0,0 +1,46 @@
+// PR tree-optimization/80635
+// { dg-do compile { target c++11 } }
+// { dg-options "-O2 -Wmaybe-uninitialized" }
+
+using size_t = decltype (sizeof (1));
+inline void *operator new (size_t, void *p) { return p; }
+template<typename T>
+struct optional
+{
+  optional () : m_dummy (), live (false) {}
+  void emplace () { new (&m_item) T (); live = true; }
+  ~optional () { if (live) m_item.~T (); }
+
+  union
+  {
+    struct {} m_dummy;
+    T m_item;
+  };
+  bool live;
+};
+
+extern int get ();
+extern void set (int);
+
+struct A
+{
+  A () : m (get ()) {}
+  ~A () { set (m); }	// { dg-bogus "may be used uninitialized in this function" }
+
+  int m;
+};
+
+struct B
+{
+  B ();
+  ~B ();
+};
+
+void func ()
+{
+  optional<A> maybe_a;
+  optional<B> maybe_b;
+
+  maybe_a.emplace ();
+  maybe_b.emplace ();
+}
--- gcc/testsuite/g++.dg/warn/pr80635-2.C.jj	2021-02-24 12:24:15.176834532 +0100
+++ gcc/testsuite/g++.dg/warn/pr80635-2.C	2021-02-24 12:24:15.176834532 +0100
@@ -0,0 +1,31 @@
+// PR tree-optimization/80635
+// { dg-do compile { target c++17 } }
+// { dg-options "-O2 -Wmaybe-uninitialized" }
+
+#include <optional>
+
+extern int get ();
+extern void set (int);
+
+struct A
+{
+  A () : m (get ()) {}
+  ~A () { set (m); }	// { dg-bogus "may be used uninitialized in this function" }
+
+  int m;
+};
+
+struct B
+{
+  B ();
+  ~B ();
+};
+
+void func ()
+{
+  std::optional<A> maybe_a;
+  std::optional<B> maybe_b;
+
+  maybe_a.emplace ();
+  maybe_b.emplace ();
+}


	Jakub

On Wed, 24 Feb 2021, Jakub Jelinek wrote:

> On Wed, Feb 24, 2021 at 11:50:10AM +0100, Richard Biener wrote:
> > > In the PR using NOP_EXPR has been discussed as one possibility and has been
> > > rejected because at expansion it will emit a superfluous & 1 operation.
> > > I still think it is a good idea to use NOP_EXPR and so have changed
> > > expansion to not emit that & 1 operation in that case.  Both spots are
> > > done with tight conditions (bool only etc.), as I'd like to fix just
> > > that case and not introduce a wider general optimization, but perhaps
> > > later we could lift it and do a general range of arbitrary
> > > type_has_mode_precision_p to non-type_has_mode_precision_p with same
> > > TYPE_MODE case.
> > 
> > But it still is a pessimization.  VCE says there's no code to be
> > generated but NOP_EXPR says there is a conversion involved, even
> > if you later elide it via ssa_name_has_boolean_range.
> 
> I'm not convinced it is a pessimization.
> Because, a NOP_EXPR is something the compiler can optimize orders of
> magnitude more than VCE.
> To back that up by some random numbers,
> grep CASE_CONVERT: gimple-match.c | wc -l; grep VIEW_CONVERT_EXPR: gimple-match.c | wc -l
> 417
> 18
> 
> > So I wonder what other optimizations are prevented here?
> 
> > Why does uninit warn with VCE but not with NOP_EXPR?  Or does the
> > warning disappear because of those other optimizations you mention?
> 
> The optimization that it prevents is in this particular case in tree-vrp.c
> (vrp_simplify_cond_using_ranges):
> 
>       if (!is_gimple_assign (def_stmt)
>           || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def_stmt)))
>         return;
> so it punts on VIEW_CONVERT_EXPR, with NOP_EXPR it optimizes that:
>   _9 = (bool) maybe_a$4_7;
>   if (_9 != 0)
> into:
>   _9 = (bool) maybe_a$4_7;
>   if (maybe_a$4_7 != 0)
> 
> Now, if I apply my patch but manually disable this
> vrp_simplify_cond_using_ranges optimization, then the uninit warning is
> back, so on the uninit side it is not about VIEW_CONVERT_EXPR vs. NOP_EXPR,
> both are bad there, uninit wants the guarding condition to be
> that SSA_NAME and not some demotion cast thereof.
> We have:
>   # maybe_a$m_6 = PHI <_5(4), maybe_a$m_4(D)(6)>
>   # maybe_a$4_7 = PHI <1(4), 0(6)>
> ...
> One of:
>   _9 = VIEW_CONVERT_EXPR<bool>(maybe_a$4_7);
>   if (_9 != 0)
> or:
>   _9 = (bool) maybe_a$4_7;
>   if (_9 != 0)
> or:
>   if (maybe_a$4_7 != 0)
> followed by:
>     goto <bb 11>; [0.00%]
>   else
>     goto <bb 14>; [0.00%]
> ...
>   <bb 11> [count: 0]:
>   set (maybe_a$m_6);
> and uninit wants to see that maybe_a$m_4(D) is not used if
> bb 11 is encountered.
> 
> So, if you are strongly opposed to the posted patch, I guess the fix can be
> (at least fixes the testcase; completely untested except for
> make check-c++-all RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} dg.exp=pr80635*.C'
> ) following.
> But, I fear there will be dozens of other spots where we'll punt on
> optimizing when it is a VCE rather than NOP_EXPR.

Yes, I don't think the folding is desired.  Since it can't be applied
in all cases anyway (ssa_name_has_boolean_range), so it's better if
passes learn to handle GIMPLEs V_C_E type-punning (we're still missing
something along (subreg:.. ) on RTL, but V_C_E with relaxed requirements
would do).

Note I'd rather do the reverse transformation to elide
bit-precision arithmetic to full mode precision one and then
communicate the not required truncations by using V_C_E
instead of NOPS from the mode precision arithmetic to the
bit-precision result.

Small comment about the patch below, which otherwise is OK:

> 2021-02-24  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/80635
> 	* tree-vrp.c (vrp_simplify_cond_using_ranges): Also handle
> 	VIEW_CONVERT_EXPR if modes are the same, innerop is integral and
> 	has mode precision.
> 
> 	* g++.dg/warn/pr80635-1.C: New test.
> 	* g++.dg/warn/pr80635-2.C: New test.
> 
> --- gcc/tree-vrp.c.jj	2021-02-24 12:56:58.573939572 +0100
> +++ gcc/tree-vrp.c	2021-02-24 13:05:22.675326780 +0100
> @@ -4390,11 +4390,24 @@ vrp_simplify_cond_using_ranges (vr_value
>        gimple *def_stmt = SSA_NAME_DEF_STMT (op0);
>        tree innerop;
>  
> -      if (!is_gimple_assign (def_stmt)
> -	  || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def_stmt)))
> +      if (!is_gimple_assign (def_stmt))
>  	return;
>  
> -      innerop = gimple_assign_rhs1 (def_stmt);
> +      switch (gimple_assign_rhs_code (def_stmt))
> +	{
> +	CASE_CONVERT:
> +	  innerop = gimple_assign_rhs1 (def_stmt);
> +	  break;
> +	case VIEW_CONVERT_EXPR:
> +	  innerop = TREE_OPERAND (gimple_assign_rhs1 (def_stmt), 0);
> +	  if (TYPE_MODE (TREE_TYPE (op0)) != TYPE_MODE (TREE_TYPE (innerop))
> +	      || !INTEGRAL_TYPE_P (TREE_TYPE (innerop))
> +	      || !type_has_mode_precision_p (TREE_TYPE (innerop)))

I think that !INTEGRAL_TYPE_P (TREE_TYPE (innerop)) is a sufficient
condition here.

Richard.

> +	    return;
> +	  break;
> +	default:
> +	  break;
> +	}
>  
>        if (TREE_CODE (innerop) == SSA_NAME
>  	  && !POINTER_TYPE_P (TREE_TYPE (innerop))
> --- gcc/testsuite/g++.dg/warn/pr80635-1.C.jj	2021-02-24 12:24:15.176834532 +0100
> +++ gcc/testsuite/g++.dg/warn/pr80635-1.C	2021-02-24 12:24:15.176834532 +0100
> @@ -0,0 +1,46 @@
> +// PR tree-optimization/80635
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-O2 -Wmaybe-uninitialized" }
> +
> +using size_t = decltype (sizeof (1));
> +inline void *operator new (size_t, void *p) { return p; }
> +template<typename T>
> +struct optional
> +{
> +  optional () : m_dummy (), live (false) {}
> +  void emplace () { new (&m_item) T (); live = true; }
> +  ~optional () { if (live) m_item.~T (); }
> +
> +  union
> +  {
> +    struct {} m_dummy;
> +    T m_item;
> +  };
> +  bool live;
> +};
> +
> +extern int get ();
> +extern void set (int);
> +
> +struct A
> +{
> +  A () : m (get ()) {}
> +  ~A () { set (m); }	// { dg-bogus "may be used uninitialized in this function" }
> +
> +  int m;
> +};
> +
> +struct B
> +{
> +  B ();
> +  ~B ();
> +};
> +
> +void func ()
> +{
> +  optional<A> maybe_a;
> +  optional<B> maybe_b;
> +
> +  maybe_a.emplace ();
> +  maybe_b.emplace ();
> +}
> --- gcc/testsuite/g++.dg/warn/pr80635-2.C.jj	2021-02-24 12:24:15.176834532 +0100
> +++ gcc/testsuite/g++.dg/warn/pr80635-2.C	2021-02-24 12:24:15.176834532 +0100
> @@ -0,0 +1,31 @@
> +// PR tree-optimization/80635
> +// { dg-do compile { target c++17 } }
> +// { dg-options "-O2 -Wmaybe-uninitialized" }
> +
> +#include <optional>
> +
> +extern int get ();
> +extern void set (int);
> +
> +struct A
> +{
> +  A () : m (get ()) {}
> +  ~A () { set (m); }	// { dg-bogus "may be used uninitialized in this function" }
> +
> +  int m;
> +};
> +
> +struct B
> +{
> +  B ();
> +  ~B ();
> +};
> +
> +void func ()
> +{
> +  std::optional<A> maybe_a;
> +  std::optional<B> maybe_b;
> +
> +  maybe_a.emplace ();
> +  maybe_b.emplace ();
> +}
> 
> 
> 	Jakub
> 
>

On 2/24/21 5:13 AM, Jakub Jelinek via Gcc-patches wrote:
> On Wed, Feb 24, 2021 at 11:50:10AM +0100, Richard Biener wrote:
>>> In the PR using NOP_EXPR has been discussed as one possibility and has been
>>> rejected because at expansion it will emit a superfluous & 1 operation.
>>> I still think it is a good idea to use NOP_EXPR and so have changed
>>> expansion to not emit that & 1 operation in that case.  Both spots are
>>> done with tight conditions (bool only etc.), as I'd like to fix just
>>> that case and not introduce a wider general optimization, but perhaps
>>> later we could lift it and do a general range of arbitrary
>>> type_has_mode_precision_p to non-type_has_mode_precision_p with same
>>> TYPE_MODE case.
>>
>> But it still is a pessimization.  VCE says there's no code to be
>> generated but NOP_EXPR says there is a conversion involved, even
>> if you later elide it via ssa_name_has_boolean_range.
> 
> I'm not convinced it is a pessimization.
> Because, a NOP_EXPR is something the compiler can optimize orders of
> magnitude more than VCE.
> To back that up by some random numbers,
> grep CASE_CONVERT: gimple-match.c | wc -l; grep VIEW_CONVERT_EXPR: gimple-match.c | wc -l
> 417
> 18
> 
>> So I wonder what other optimizations are prevented here?
> 
>> Why does uninit warn with VCE but not with NOP_EXPR?  Or does the
>> warning disappear because of those other optimizations you mention?

Can you comment on Jeff's POC patch in the PR?  Would it make sense
to apply it (with adjustments if necessary) as well to make the warning
more robust in case the VCE comes back?

Martin

> 
> The optimization that it prevents is in this particular case in tree-vrp.c
> (vrp_simplify_cond_using_ranges):
> 
>        if (!is_gimple_assign (def_stmt)
>            || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def_stmt)))
>          return;
> so it punts on VIEW_CONVERT_EXPR, with NOP_EXPR it optimizes that:
>    _9 = (bool) maybe_a$4_7;
>    if (_9 != 0)
> into:
>    _9 = (bool) maybe_a$4_7;
>    if (maybe_a$4_7 != 0)
> 
> Now, if I apply my patch but manually disable this
> vrp_simplify_cond_using_ranges optimization, then the uninit warning is
> back, so on the uninit side it is not about VIEW_CONVERT_EXPR vs. NOP_EXPR,
> both are bad there, uninit wants the guarding condition to be
> that SSA_NAME and not some demotion cast thereof.
> We have:
>    # maybe_a$m_6 = PHI <_5(4), maybe_a$m_4(D)(6)>
>    # maybe_a$4_7 = PHI <1(4), 0(6)>
> ...
> One of:
>    _9 = VIEW_CONVERT_EXPR<bool>(maybe_a$4_7);
>    if (_9 != 0)
> or:
>    _9 = (bool) maybe_a$4_7;
>    if (_9 != 0)
> or:
>    if (maybe_a$4_7 != 0)
> followed by:
>      goto <bb 11>; [0.00%]
>    else
>      goto <bb 14>; [0.00%]
> ...
>    <bb 11> [count: 0]:
>    set (maybe_a$m_6);
> and uninit wants to see that maybe_a$m_4(D) is not used if
> bb 11 is encountered.
> 
> So, if you are strongly opposed to the posted patch, I guess the fix can be
> (at least fixes the testcase; completely untested except for
> make check-c++-all RUNTESTFLAGS='--target_board=unix\{-m32,-m64\} dg.exp=pr80635*.C'
> ) following.
> But, I fear there will be dozens of other spots where we'll punt on
> optimizing when it is a VCE rather than NOP_EXPR.
> 
> 2021-02-24  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR tree-optimization/80635
> 	* tree-vrp.c (vrp_simplify_cond_using_ranges): Also handle
> 	VIEW_CONVERT_EXPR if modes are the same, innerop is integral and
> 	has mode precision.
> 
> 	* g++.dg/warn/pr80635-1.C: New test.
> 	* g++.dg/warn/pr80635-2.C: New test.
> 
> --- gcc/tree-vrp.c.jj	2021-02-24 12:56:58.573939572 +0100
> +++ gcc/tree-vrp.c	2021-02-24 13:05:22.675326780 +0100
> @@ -4390,11 +4390,24 @@ vrp_simplify_cond_using_ranges (vr_value
>         gimple *def_stmt = SSA_NAME_DEF_STMT (op0);
>         tree innerop;
>   
> -      if (!is_gimple_assign (def_stmt)
> -	  || !CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def_stmt)))
> +      if (!is_gimple_assign (def_stmt))
>   	return;
>   
> -      innerop = gimple_assign_rhs1 (def_stmt);
> +      switch (gimple_assign_rhs_code (def_stmt))
> +	{
> +	CASE_CONVERT:
> +	  innerop = gimple_assign_rhs1 (def_stmt);
> +	  break;
> +	case VIEW_CONVERT_EXPR:
> +	  innerop = TREE_OPERAND (gimple_assign_rhs1 (def_stmt), 0);
> +	  if (TYPE_MODE (TREE_TYPE (op0)) != TYPE_MODE (TREE_TYPE (innerop))
> +	      || !INTEGRAL_TYPE_P (TREE_TYPE (innerop))
> +	      || !type_has_mode_precision_p (TREE_TYPE (innerop)))
> +	    return;
> +	  break;
> +	default:
> +	  break;
> +	}
>   
>         if (TREE_CODE (innerop) == SSA_NAME
>   	  && !POINTER_TYPE_P (TREE_TYPE (innerop))
> --- gcc/testsuite/g++.dg/warn/pr80635-1.C.jj	2021-02-24 12:24:15.176834532 +0100
> +++ gcc/testsuite/g++.dg/warn/pr80635-1.C	2021-02-24 12:24:15.176834532 +0100
> @@ -0,0 +1,46 @@
> +// PR tree-optimization/80635
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-O2 -Wmaybe-uninitialized" }
> +
> +using size_t = decltype (sizeof (1));
> +inline void *operator new (size_t, void *p) { return p; }
> +template<typename T>
> +struct optional
> +{
> +  optional () : m_dummy (), live (false) {}
> +  void emplace () { new (&m_item) T (); live = true; }
> +  ~optional () { if (live) m_item.~T (); }
> +
> +  union
> +  {
> +    struct {} m_dummy;
> +    T m_item;
> +  };
> +  bool live;
> +};
> +
> +extern int get ();
> +extern void set (int);
> +
> +struct A
> +{
> +  A () : m (get ()) {}
> +  ~A () { set (m); }	// { dg-bogus "may be used uninitialized in this function" }
> +
> +  int m;
> +};
> +
> +struct B
> +{
> +  B ();
> +  ~B ();
> +};
> +
> +void func ()
> +{
> +  optional<A> maybe_a;
> +  optional<B> maybe_b;
> +
> +  maybe_a.emplace ();
> +  maybe_b.emplace ();
> +}
> --- gcc/testsuite/g++.dg/warn/pr80635-2.C.jj	2021-02-24 12:24:15.176834532 +0100
> +++ gcc/testsuite/g++.dg/warn/pr80635-2.C	2021-02-24 12:24:15.176834532 +0100
> @@ -0,0 +1,31 @@
> +// PR tree-optimization/80635
> +// { dg-do compile { target c++17 } }
> +// { dg-options "-O2 -Wmaybe-uninitialized" }
> +
> +#include <optional>
> +
> +extern int get ();
> +extern void set (int);
> +
> +struct A
> +{
> +  A () : m (get ()) {}
> +  ~A () { set (m); }	// { dg-bogus "may be used uninitialized in this function" }
> +
> +  int m;
> +};
> +
> +struct B
> +{
> +  B ();
> +  ~B ();
> +};
> +
> +void func ()
> +{
> +  std::optional<A> maybe_a;
> +  std::optional<B> maybe_b;
> +
> +  maybe_a.emplace ();
> +  maybe_b.emplace ();
> +}
> 
> 
> 	Jakub
>

On Wed, Feb 24, 2021 at 05:54:08PM -0700, Martin Sebor via Gcc-patches wrote:
> > > Why does uninit warn with VCE but not with NOP_EXPR?  Or does the
> > > warning disappear because of those other optimizations you mention?
> 
> Can you comment on Jeff's POC patch in the PR?  Would it make sense
> to apply it (with adjustments if necessary) as well to make the warning
> more robust in case the VCE comes back?

It has been a while since I looked into uninit pass in detail, so I don't
know if it is the right spot to do it, but as my mail clearly states,
it is not just about VIEW_CONVERT_EXPR, demoting NOP_EXPR/CONVERT_EXPR
that vrp_simplify_cond_using_ranges doesn't simplify are the same case.

So, if the uninit pass was to be made robust against tree-vrp.c not doing
its job, then it would basically need to virtually repeat
vrp_simplify_cond_using_ranges (using either get_range_info or ranger).
I'm not convinced it is the right way though to account for possibilities
of disabled passes by repeating their code in other passes.

	Jakub