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 <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)))
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
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
