diff mbox

Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

Message ID 20151125143509.GU21807@redhat.com
State New
Headers show

Commit Message

Marek Polacek Nov. 25, 2015, 2:35 p.m. UTC
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.  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?

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2015-11-25  Marek Polacek  <polacek@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.


	Marek

Comments

Richard Biener Nov. 25, 2015, 2:45 p.m. UTC | #1
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  <polacek@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
> 
>
Jakub Jelinek Nov. 25, 2015, 2:46 p.m. UTC | #2
On Wed, Nov 25, 2015 at 03:35:10PM +0100, Marek Polacek wrote:
> 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.

Ran into similar issues many times in the past, the main issue is that the C
FE wants that before c_fully_fold is called on some expression that nobody
wraps anything into a SAVE_EXPR except by calling c_save_expr.
So, unless we get rid of that (severe) limitation, we should make sure
that either match.pd never creates SAVE_EXPRs when called from the "early"
spots in the C FE, or that it uses c_save_expr instead of save_expr.
But finding out if you have an expression on which c_fully_fold has been
already called or not is non-trivial too.

Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say
whether c_fully_fold_internal has already processed it or not, and just
get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but
only if that bit is not yet already set, and set it afterwards.

	Jakub
Richard Biener Nov. 25, 2015, 2:55 p.m. UTC | #3
On Wed, 25 Nov 2015, Jakub Jelinek wrote:

> On Wed, Nov 25, 2015 at 03:35:10PM +0100, Marek Polacek wrote:
> > 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.
> 
> Ran into similar issues many times in the past, the main issue is that the C
> FE wants that before c_fully_fold is called on some expression that nobody
> wraps anything into a SAVE_EXPR except by calling c_save_expr.
> So, unless we get rid of that (severe) limitation, we should make sure
> that either match.pd never creates SAVE_EXPRs when called from the "early"
> spots in the C FE, or that it uses c_save_expr instead of save_expr.
> But finding out if you have an expression on which c_fully_fold has been
> already called or not is non-trivial too.
> 
> Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say
> whether c_fully_fold_internal has already processed it or not, and just
> get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but
> only if that bit is not yet already set, and set it afterwards.

c_gimplify_expr and SAVE_EXPR_RESOLVED_P would work as well I guess?

Richard.
Richard Biener Nov. 25, 2015, 2:56 p.m. UTC | #4
On Wed, 25 Nov 2015, Richard Biener wrote:

> On Wed, 25 Nov 2015, Jakub Jelinek wrote:
> 
> > On Wed, Nov 25, 2015 at 03:35:10PM +0100, Marek Polacek wrote:
> > > 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.
> > 
> > Ran into similar issues many times in the past, the main issue is that the C
> > FE wants that before c_fully_fold is called on some expression that nobody
> > wraps anything into a SAVE_EXPR except by calling c_save_expr.
> > So, unless we get rid of that (severe) limitation, we should make sure
> > that either match.pd never creates SAVE_EXPRs when called from the "early"
> > spots in the C FE, or that it uses c_save_expr instead of save_expr.
> > But finding out if you have an expression on which c_fully_fold has been
> > already called or not is non-trivial too.
> > 
> > Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say
> > whether c_fully_fold_internal has already processed it or not, and just
> > get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but
> > only if that bit is not yet already set, and set it afterwards.
> 
> c_gimplify_expr and SAVE_EXPR_RESOLVED_P would work as well I guess?

Or simply not call fold () from convert () -> convert_to_integer ().

Richard.
Marek Polacek Nov. 25, 2015, 2:56 p.m. UTC | #5
On Wed, Nov 25, 2015 at 03:45:08PM +0100, Richard Biener wrote:
> But the whole point of the SAVE_EXPR is that it does _not_ "duplicate" it,
> it just creates another use of the same value.

Of course, but when 'x' in that pattern doesn't have side-effects, it's not
wrapped in SAVE_EXPR and gets duplicated, generating unnecessary code, this
is when I think the pattern is harmful.
 
> No.  If c_fully_fold can't handle SAVE_EXPRs then maybe c_gimplify_expr
> can simply strip them.

Uhm, can we just strip SAVE_EXPRs like that?  That sounds wrong.  Did you
mean C_MAYBE_CONST_EXPR?

	Marek
Marek Polacek Nov. 25, 2015, 2:59 p.m. UTC | #6
On Wed, Nov 25, 2015 at 03:56:39PM +0100, Richard Biener wrote:
> > c_gimplify_expr and SAVE_EXPR_RESOLVED_P would work as well I guess?
> 
> Or simply not call fold () from convert () -> convert_to_integer ().

I was thinking about calling convert_to_integer_nofold in convert; that
fixed the testcase I think, but I haven't done a proper testing.

	Marek
Joseph Myers Nov. 25, 2015, 3:16 p.m. UTC | #7
On Wed, 25 Nov 2015, Jakub Jelinek wrote:

> But finding out if you have an expression on which c_fully_fold has been
> already called or not is non-trivial too.

The rule is that if the C front end needs to fold early for any reason 
(e.g. for warnings) then the result of folding gets wrapped in a 
C_MAYBE_CONST_EXPR as a signal that what's inside there doesn't need 
folding again - or in another kind of tree that also isn't folded inside, 
such as SAVE_EXPR or CALL_EXPR.

> Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say
> whether c_fully_fold_internal has already processed it or not, and just
> get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but
> only if that bit is not yet already set, and set it afterwards.

I suppose you could do that, in line with the general principle of 
reducing early folding (as long as you ensure that folding the contents of 
a SAVE_EXPR results in modifying that SAVE_EXPR so that all pointers to it 
stay pointing to the same tree node).
Marek Polacek Nov. 26, 2015, 11:15 a.m. UTC | #8
On Wed, Nov 25, 2015 at 03:16:53PM +0000, Joseph Myers wrote:
> > Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say
> > whether c_fully_fold_internal has already processed it or not, and just
> > get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but
> > only if that bit is not yet already set, and set it afterwards.
> 
> I suppose you could do that, in line with the general principle of 
> reducing early folding (as long as you ensure that folding the contents of 
> a SAVE_EXPR results in modifying that SAVE_EXPR so that all pointers to it 
> stay pointing to the same tree node).

I had a go at this, but I'm now skeptical about removing c_save_expr.
save_expr calls fold (), so we need to ensure that we don't pass any
C_MAYBE_CONST_EXPRs into it, meaning that we'd need to call c_fully_fold before
save_expr anyway...

So maybe go the "remove C_MAYBE_CONST_EXPRs in SAVE_EXPRs in c_gimplify_expr"
way?

	Marek
Jakub Jelinek Nov. 26, 2015, 11:26 a.m. UTC | #9
On Thu, Nov 26, 2015 at 12:15:48PM +0100, Marek Polacek wrote:
> On Wed, Nov 25, 2015 at 03:16:53PM +0000, Joseph Myers wrote:
> > > Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say
> > > whether c_fully_fold_internal has already processed it or not, and just
> > > get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but
> > > only if that bit is not yet already set, and set it afterwards.
> > 
> > I suppose you could do that, in line with the general principle of 
> > reducing early folding (as long as you ensure that folding the contents of 
> > a SAVE_EXPR results in modifying that SAVE_EXPR so that all pointers to it 
> > stay pointing to the same tree node).
> 
> I had a go at this, but I'm now skeptical about removing c_save_expr.
> save_expr calls fold (), so we need to ensure that we don't pass any
> C_MAYBE_CONST_EXPRs into it, meaning that we'd need to call c_fully_fold before
> save_expr anyway...

I bet that is too hard for stage3, but IMHO if we want delayed folding, we
want to delay it even for SAVE_EXPRs.  Supposedly the reason why save_expr
calls fold is to determine the cases where there is no point to create the
SAVE_EXPR.  But perhaps it should just fold for the purpose of testing that
and return the original expression if after folding it is simple
arithmetics, and wrap the original expression into SAVE_EXPR.

Though in this particular case, where save_expr is just an optimization it
is perhaps premature optimization and we should not perform that.

For stage3, I agree, some other fix is needed (and one usable also for the 5
branch).

	Jakub
Richard Biener Nov. 26, 2015, 11:31 a.m. UTC | #10
On Thu, 26 Nov 2015, Jakub Jelinek wrote:

> On Thu, Nov 26, 2015 at 12:15:48PM +0100, Marek Polacek wrote:
> > On Wed, Nov 25, 2015 at 03:16:53PM +0000, Joseph Myers wrote:
> > > > Wonder if we couldn't use some FE specific bit on the SAVE_EXPR to say
> > > > whether c_fully_fold_internal has already processed it or not, and just
> > > > get rid of c_save_expr, in c_fully_fold* recurse into SAVE_EXPRs too, but
> > > > only if that bit is not yet already set, and set it afterwards.
> > > 
> > > I suppose you could do that, in line with the general principle of 
> > > reducing early folding (as long as you ensure that folding the contents of 
> > > a SAVE_EXPR results in modifying that SAVE_EXPR so that all pointers to it 
> > > stay pointing to the same tree node).
> > 
> > I had a go at this, but I'm now skeptical about removing c_save_expr.
> > save_expr calls fold (), so we need to ensure that we don't pass any
> > C_MAYBE_CONST_EXPRs into it, meaning that we'd need to call c_fully_fold before
> > save_expr anyway...
> 
> I bet that is too hard for stage3, but IMHO if we want delayed folding, we
> want to delay it even for SAVE_EXPRs.  Supposedly the reason why save_expr
> calls fold is to determine the cases where there is no point to create the
> SAVE_EXPR.  But perhaps it should just fold for the purpose of testing that
> and return the original expression if after folding it is simple
> arithmetics, and wrap the original expression into SAVE_EXPR.

Btw, I tried to remove that fold () at some point but it spectacularly
regressed (though before the C++ early folding work) in constexpr
cases.

> Though in this particular case, where save_expr is just an optimization it
> is perhaps premature optimization and we should not perform that.
> 
> For stage3, I agree, some other fix is needed (and one usable also for the 5
> branch).

I'm currently testing the genmatch.c patch ... (which might make this
situation even worse).  I don't think we have the issue on the 5 branch
so much (because of way less patterns in match.pd).

Richard.
Joseph Myers Nov. 26, 2015, 12:34 p.m. UTC | #11
On Thu, 26 Nov 2015, Marek Polacek wrote:

> I had a go at this, but I'm now skeptical about removing c_save_expr.
> save_expr calls fold (), so we need to ensure that we don't pass any
> C_MAYBE_CONST_EXPRs into it, meaning that we'd need to call c_fully_fold before
> save_expr anyway...
> 
> So maybe go the "remove C_MAYBE_CONST_EXPRs in SAVE_EXPRs in c_gimplify_expr"
> way?

I believe it should be safe for gimplification to process 
C_MAYBE_CONST_EXPR in the same way c_fully_fold_internal does.  That is, 
this should not affect correctness.  If a C_MAYBE_CONST_EXPR got through 
to gimplification, in some cases it may mean that something did not get 
properly folded with c_fully_fold as it should have done - but if the move 
to match.pd means all optimizations currently done with fold end up 
working on GIMPLE as well, any missed optimizations from this should 
disappear (and if we can solve the diagnostics issues, eventually fewer 
calls to c_fully_fold should be needed and they should be more about 
checking what can occur in constant expressions and less about folding for 
optimization).

The general principle of delaying folding also means that we should move 
away from convert_* folding things.
diff mbox

Patch

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);
+}