[PATCH] match.pd, expand: Fold VCE from integer with [0, 1] range to bool into NOP_EXPR [PR80635]
Martin Sebor
msebor@gmail.com
Thu Feb 25 00:54:08 GMT 2021
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
>
More information about the Gcc-patches
mailing list