On Wed, 25 Nov 2015, Marek Polacek wrote:
> This is a bit tangled and I don't know how to best fix this so let me explain
> in more detail.
>
> The gist is that a C_MAYBE_CONST_EXPR leaks into the gimplifier.
>
> In the testcase, we have
>
> (short) ((i ? *e : 0) & ~u | i & u);
>
> where 'e' is a pointer to volatile. During c_parser_conditional_expression,
> we
> wrap 'i' and '*e' in C_MAYBE_CONST_EXPR. Later on, we get to build_c_cast,
> and
> here we're trying to cast
>
> (c_maybe_const_expr <i> != 0 ? (unsigned int) c_maybe_const_expr <*e > : 0)
> & ~u | (unsigned int) i & u
>
> to "short", so we call convert()->convert_to_integer() etc. As a part of this
> conversion, we try to fold the expr. Now, we match this pattern:
>
> (x & ~m) | (y & m) -> ((x ^ y) & m) ^ x
>
> Look how 'x' is duplicated in the result of the pattern, and since (because of
> the volatile 'e') it has TREE_SIDE_EFFECTS, we need to wrap it in a SAVE_EXPR,
> which means the convert() produces
>
> (short int) (((SAVE_EXPR <c_maybe_const_expr <i> != 0 ? (unsigned short)
> c_maybe_const_expr <*e > : 0>)
> ^ (unsigned short) i) & (unsigned short) u
> ^ (SAVE_EXPR <c_maybe_const_expr <i > != 0 ? (unsigned short)
> c_maybe_const_expr <*e > : 0>))
>
> What's important is that we end up with a C_MAYBE_CONST_EXPR in a SAVE_EXPR.
>
> Down the line, we get into c_process_expr_stmt, where there's
>
> expr = c_fully_fold (expr, false, NULL);
>
> so we try to fully-fold the whole expression. However, c_fully_fold just
> gives up when it sees a SAVE_EXPR, so it doesn't scrub out C_MAYBE_CONST_EXPR.
> It then leaks into the gimplifier and crashes.
>
> This pattern is probably the only one that is problematical in this regard.
> Looking more at this pattern, it looks dubious, because imagine the 'x' in
> the pattern is something complex, e.g. a huge COND_EXPR or somesuch -- in
> that case duplicating the 'x' does more harm than good.
But the whole point of the SAVE_EXPR is that it does _not_ "duplicate" it,
it just creates another use of the same value.
> So after all, I
> think this transformation should be restricted a bit, and only kick in when
> 'x' is a constant, an SSA_NAME, or a certain DECL_P. Seems simple_operand_p
> in fold-const.c was what I was after, so I've just used that.
>
> With this patch, we won't create those SAVE_EXPRs so c_fully_fold is able to
> get rid of the C_MAYBE_CONST_EXPRs and the gimplifier is happy.
>
> Thoughts?
No. If c_fully_fold can't handle SAVE_EXPRs then maybe c_gimplify_expr
can simply strip them.
Richard.
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2015-11-25 Marek Polacek <pola...@redhat.com>
>
> PR c/68513
> * fold-const.c (simple_operand_p): Export.
> * fold-const.h (simple_operand_p): Declare.
> * match.pd ((x & ~m) | (y & m)): Guard with simple_operand_p.
>
> * gcc.dg/torture/pr68513.c: New test.
>
> diff --git gcc/fold-const.c gcc/fold-const.c
> index 698062e..9bb3a53 100644
> --- gcc/fold-const.c
> +++ gcc/fold-const.c
> @@ -122,7 +122,6 @@ static tree decode_field_reference (location_t, tree,
> HOST_WIDE_INT *,
> HOST_WIDE_INT *,
> machine_mode *, int *, int *, int *,
> tree *, tree *);
> -static int simple_operand_p (const_tree);
> static bool simple_operand_p_2 (tree);
> static tree range_binop (enum tree_code, tree, tree, int, tree, int);
> static tree range_predecessor (tree);
> @@ -4059,10 +4058,10 @@ sign_bit_p (tree exp, const_tree val)
> return NULL_TREE;
> }
>
> -/* Subroutine for fold_truth_andor_1: determine if an operand is simple
> enough
> - to be evaluated unconditionally. */
> +/* Determine if an operand is simple enough to be evaluated
> + unconditionally. */
>
> -static int
> +bool
> simple_operand_p (const_tree exp)
> {
> /* Strip any conversions that don't change the machine mode. */
> diff --git gcc/fold-const.h gcc/fold-const.h
> index 7741802..717c840 100644
> --- gcc/fold-const.h
> +++ gcc/fold-const.h
> @@ -181,6 +181,7 @@ extern tree const_unop (enum tree_code, tree, tree);
> extern tree const_binop (enum tree_code, tree, tree, tree);
> extern bool negate_mathfn_p (combined_fn);
> extern const char *c_getstr (tree);
> +extern bool simple_operand_p (const_tree);
>
> /* Return OFF converted to a pointer offset type suitable as offset for
> POINTER_PLUS_EXPR. Use location LOC for this conversion. */
> diff --git gcc/match.pd gcc/match.pd
> index e86cc8b..4aee4a6 100644
> --- gcc/match.pd
> +++ gcc/match.pd
> @@ -877,7 +877,8 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
> /* (x & ~m) | (y & m) -> ((x ^ y) & m) ^ x */
> (simplify
> (bit_ior:c (bit_and:cs @0 (bit_not @2)) (bit_and:cs @1 @2))
> - (bit_xor (bit_and (bit_xor @0 @1) @2) @0))
> + (if (simple_operand_p (@0))
> + (bit_xor (bit_and (bit_xor @0 @1) @2) @0)))
>
> /* Fold A - (A & B) into ~B & A. */
> (simplify
> diff --git gcc/testsuite/gcc.dg/torture/pr68513.c
> gcc/testsuite/gcc.dg/torture/pr68513.c
> index e69de29..4e08b29 100644
> --- gcc/testsuite/gcc.dg/torture/pr68513.c
> +++ gcc/testsuite/gcc.dg/torture/pr68513.c
> @@ -0,0 +1,13 @@
> +/* PR c/68513 */
> +/* { dg-do compile } */
> +
> +int i;
> +unsigned u;
> +volatile unsigned int *e;
> +
> +void
> +fn1 (void)
> +{
> + (short) ((i ? *e : 0) & ~u | i & u);
> + (short) (((0, 0) ? *e : 0) & ~u | i & u);
> +}
>
> Marek
>
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
