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