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