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

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

[Marek Polacek]

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.

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

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Richard Biener]

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
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Jakub Jelinek]

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

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Richard Biener]

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.

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Richard Biener]

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.

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Marek Polacek]

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

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Marek Polacek]

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

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Richard Biener]

On Wed, 25 Nov 2015, Marek Polacek wrote:

> 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.

Ok, I see what you mean.  Yes, genmatch only inserts save_exprs
when required for correctness.  But we can easily change that, making
the c_fully_fold issue possibly worse of course.

Index: gcc/genmatch.c
===================================================================
--- gcc/genmatch.c      (revision 230858)
+++ gcc/genmatch.c      (working copy)
@@ -3112,16 +3111,10 @@ dt_simplify::gen_1 (FILE *f, int indent,
              {
                if (cinfo.info[i].same_as != (unsigned)i)
                  continue;
-               if (!cinfo.info[i].force_no_side_effects_p
-                   && cinfo.info[i].result_use_count > 1)
-                 {
-                   fprintf_indent (f, indent,
-                                   "if (TREE_SIDE_EFFECTS 
(captures[%d]))\n",
-                                   i);
-                   fprintf_indent (f, indent,
-                                   "  captures[%d] = save_expr 
(captures[%d]);\n",
-                                   i, i);
-                 }
+               if (cinfo.info[i].result_use_count > 1)
+                 fprintf_indent (f, indent,
+                                 "captures[%d] = save_expr 
(captures[%d]);\n",
+                                 i, i);
              }
          for (unsigned j = 0; j < e->ops.length (); ++j)
            {

> > 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?

Yes, strip C_MAYBE_CONST_EXPR but inside SAVE_EXPRs of course.

Richard.

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Joseph Myers]

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).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Marek Polacek]

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

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Jakub Jelinek]

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

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Richard Biener]

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.

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Joseph Myers]

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.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Marek Polacek]

On Thu, Nov 26, 2015 at 12:34:55PM +0000, Joseph Myers wrote:
> 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).
 
So here's an attempt to strip C_MAYBE_CONST_EXPRs, only for SAVE_EXPRs, because
c_fully_fold in c_process_stmt_expr should deal with other expressions.

My worry was of course C_MAYBE_CONST_EXPR_PRE.  But it seems we'll never have
any at that point, since it's already been processed and transformed to a
COMPOUND_EXPR.  But do I like this patch?  No.

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

Yep, I tried using _nofold variants, but it had soem fallout.  Anyway,
something for next stage1.

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

2015-11-26  Marek Polacek  <polacek@redhat.com>

	PR c/68513
	* c-gimplify.c (strip_c_maybe_const_expr_r): New.
	(c_gimplify_expr): Call it.

	* gcc.dg/torture/pr68513.c: New test.

diff --git gcc/c-family/c-gimplify.c gcc/c-family/c-gimplify.c
index fc4a44a..c096575 100644
--- gcc/c-family/c-gimplify.c
+++ gcc/c-family/c-gimplify.c
@@ -212,6 +212,21 @@ c_build_bind_expr (location_t loc, tree block, tree body)
 
 /* Gimplification of expression trees.  */
 
+/* Callback for c_gimplify_expr.  Strip C_MAYBE_CONST_EXPRs in TP so that
+   they don't leak into the middle end.  */
+
+static tree
+strip_c_maybe_const_expr_r (tree *tp, int *walk_subtrees, void *)
+{
+  if (TREE_CODE (*tp) == C_MAYBE_CONST_EXPR)
+    {
+      gcc_assert (C_MAYBE_CONST_EXPR_PRE (*tp) == NULL_TREE);
+      *tp = C_MAYBE_CONST_EXPR_EXPR (*tp);
+      *walk_subtrees = 0;
+    }
+  return NULL_TREE;
+}
+
 /* Do C-specific gimplification on *EXPR_P.  PRE_P and POST_P are as in
    gimplify_expr.  */
 
@@ -296,6 +311,10 @@ c_gimplify_expr (tree *expr_p, gimple_seq *pre_p ATTRIBUTE_UNUSED,
 	  return (enum gimplify_status) gimplify_cilk_spawn (expr_p);
 	}
 
+    case SAVE_EXPR:
+      walk_tree_without_duplicates (expr_p, strip_c_maybe_const_expr_r, NULL);
+      break;
+
     default:;
     }
 
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

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Jakub Jelinek]

On Thu, Nov 26, 2015 at 05:10:26PM +0100, Marek Polacek wrote:
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
> 
> 2015-11-26  Marek Polacek  <polacek@redhat.com>
> 
> 	PR c/68513
> 	* c-gimplify.c (strip_c_maybe_const_expr_r): New.
> 	(c_gimplify_expr): Call it.
> 
> 	* gcc.dg/torture/pr68513.c: New test.
> 
> diff --git gcc/c-family/c-gimplify.c gcc/c-family/c-gimplify.c
> index fc4a44a..c096575 100644
> --- gcc/c-family/c-gimplify.c
> +++ gcc/c-family/c-gimplify.c
> @@ -212,6 +212,21 @@ c_build_bind_expr (location_t loc, tree block, tree body)
>  
>  /* Gimplification of expression trees.  */
>  
> +/* Callback for c_gimplify_expr.  Strip C_MAYBE_CONST_EXPRs in TP so that
> +   they don't leak into the middle end.  */
> +
> +static tree
> +strip_c_maybe_const_expr_r (tree *tp, int *walk_subtrees, void *)
> +{
> +  if (TREE_CODE (*tp) == C_MAYBE_CONST_EXPR)
> +    {
> +      gcc_assert (C_MAYBE_CONST_EXPR_PRE (*tp) == NULL_TREE);
> +      *tp = C_MAYBE_CONST_EXPR_EXPR (*tp);
> +      *walk_subtrees = 0;
> +    }
> +  return NULL_TREE;
> +}
> +
>  /* Do C-specific gimplification on *EXPR_P.  PRE_P and POST_P are as in
>     gimplify_expr.  */
>  
> @@ -296,6 +311,10 @@ c_gimplify_expr (tree *expr_p, gimple_seq *pre_p ATTRIBUTE_UNUSED,
>  	  return (enum gimplify_status) gimplify_cilk_spawn (expr_p);
>  	}
>  
> +    case SAVE_EXPR:
> +      walk_tree_without_duplicates (expr_p, strip_c_maybe_const_expr_r, NULL);

Shouldn't this be done only if (!SAVE_EXPR_RESOLVED_P (*expr_p))?
Otherwise I fear bad compile time complexity, consider huge tree containing
lots of nested SAVE_EXPRs and every SAVE_EXPR appearing twice or more times
somewhere in the tree.  Perhaps the walk should also *walk_subtrees = 0;
for SAVE_EXPRs and start walking &TREE_OPERAND (*expr_p, 0) instead of
expr_p.  The nested SAVE_EXPRs would be handled when those are gimplified.

	Jakub

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Joseph Myers]

On Thu, 26 Nov 2015, Marek Polacek wrote:

> My worry was of course C_MAYBE_CONST_EXPR_PRE.  But it seems we'll never have
> any at that point, since it's already been processed and transformed to a
> COMPOUND_EXPR.  But do I like this patch?  No.

It's not obvious to me that it will always have been transformed - if a 
C_MAYBE_CONST_EXPR has escaped to gimplification, why shouldn't it be one 
with C_MAYBE_CONST_EXPR_PRE?

Also, on further consideration: the folding via c_fully_fold is relied 
upon to get information about whether an expression contains anything that 
cannot occur in an evaluated part of a constant expression / outside 
sizeof in a constant expression in C90 mode.  So if a SAVE_EXPR is created 
by language-independent code, c_fully_fold doesn't see inside it and you 
lose that information.  What that says to me is that maybe a better 
interim solution would be a lang hook for the folders to use to call 
c_save_expr instead of save_expr.  And then longer term: (a) maybe any 
folding that involves duplicating expressions and so needs to create 
SAVE_EXPRs would better be done only at the GIMPLE level, and (b) folding 
for conversions should be delayed as much as possible like other folding 
(and optimizations of conversions should move from convert.c to match.pd).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Jakub Jelinek]

On Thu, Nov 26, 2015 at 04:36:34PM +0000, Joseph Myers wrote:
> On Thu, 26 Nov 2015, Marek Polacek wrote:
> 
> > My worry was of course C_MAYBE_CONST_EXPR_PRE.  But it seems we'll never have
> > any at that point, since it's already been processed and transformed to a
> > COMPOUND_EXPR.  But do I like this patch?  No.
> 
> It's not obvious to me that it will always have been transformed - if a 
> C_MAYBE_CONST_EXPR has escaped to gimplification, why shouldn't it be one 
> with C_MAYBE_CONST_EXPR_PRE?
> 
> Also, on further consideration: the folding via c_fully_fold is relied 
> upon to get information about whether an expression contains anything that 
> cannot occur in an evaluated part of a constant expression / outside 
> sizeof in a constant expression in C90 mode.  So if a SAVE_EXPR is created 
> by language-independent code, c_fully_fold doesn't see inside it and you 
> lose that information.  What that says to me is that maybe a better 
> interim solution would be a lang hook for the folders to use to call 
> c_save_expr instead of save_expr.  And then longer term: (a) maybe any 
> folding that involves duplicating expressions and so needs to create 

But the condition whether to call c_save_expr or whether to call save_expr
instead is not constant in the C FE.
If c_fully_fold is expected to be called on the expression, then c_save_expr
needs to be used, otherwise save_expr.
Can we rely on in_late_binary_op for that?

> SAVE_EXPRs would better be done only at the GIMPLE level, and (b) folding 
> for conversions should be delayed as much as possible like other folding 
> (and optimizations of conversions should move from convert.c to match.pd).

	Jakub

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Joseph Myers]

On Thu, 26 Nov 2015, Jakub Jelinek wrote:

> > Also, on further consideration: the folding via c_fully_fold is relied 
> > upon to get information about whether an expression contains anything that 
> > cannot occur in an evaluated part of a constant expression / outside 
> > sizeof in a constant expression in C90 mode.  So if a SAVE_EXPR is created 
> > by language-independent code, c_fully_fold doesn't see inside it and you 
> > lose that information.  What that says to me is that maybe a better 
> > interim solution would be a lang hook for the folders to use to call 
> > c_save_expr instead of save_expr.  And then longer term: (a) maybe any 
> > folding that involves duplicating expressions and so needs to create 
> 
> But the condition whether to call c_save_expr or whether to call save_expr
> instead is not constant in the C FE.
> If c_fully_fold is expected to be called on the expression, then c_save_expr
> needs to be used, otherwise save_expr.
> Can we rely on in_late_binary_op for that?

Yes, I think so.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Jakub Jelinek]

On Thu, Nov 26, 2015 at 05:05:09PM +0000, Joseph Myers wrote:
> On Thu, 26 Nov 2015, Jakub Jelinek wrote:
> 
> > > Also, on further consideration: the folding via c_fully_fold is relied 
> > > upon to get information about whether an expression contains anything that 
> > > cannot occur in an evaluated part of a constant expression / outside 
> > > sizeof in a constant expression in C90 mode.  So if a SAVE_EXPR is created 
> > > by language-independent code, c_fully_fold doesn't see inside it and you 
> > > lose that information.  What that says to me is that maybe a better 
> > > interim solution would be a lang hook for the folders to use to call 
> > > c_save_expr instead of save_expr.  And then longer term: (a) maybe any 
> > > folding that involves duplicating expressions and so needs to create 
> > 
> > But the condition whether to call c_save_expr or whether to call save_expr
> > instead is not constant in the C FE.
> > If c_fully_fold is expected to be called on the expression, then c_save_expr
> > needs to be used, otherwise save_expr.
> > Can we rely on in_late_binary_op for that?
> 
> Yes, I think so.

It seems only to be set temporarily when calling convert*, then reset
back, while we supposedly want to use save_expr instead of c_save_expr also
at the point where we are genericizing, or gimplifying etc.

	Jakub

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Joseph Myers]

On Thu, 26 Nov 2015, Jakub Jelinek wrote:

> > > But the condition whether to call c_save_expr or whether to call save_expr
> > > instead is not constant in the C FE.
> > > If c_fully_fold is expected to be called on the expression, then c_save_expr
> > > needs to be used, otherwise save_expr.
> > > Can we rely on in_late_binary_op for that?
> > 
> > Yes, I think so.
> 
> It seems only to be set temporarily when calling convert*, then reset
> back, while we supposedly want to use save_expr instead of c_save_expr also
> at the point where we are genericizing, or gimplifying etc.

OK, then we may need a new flag that indicates that we are doing some 
processing after all the constant expression checks from parsing have been 
done.  (With an eventual preference that anything creating SAVE_EXPRs at 
all should happen at that late stage - that the front-end representation 
created during parsing should be closer to the source, including not 
needing to refer more than once to a given source expression, and 
genericizing / gimplifying / some such lowering step handling semantics 
that require SAVE_EXPRs.)

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Fix pattern causing C_MAYBE_CONST_EXPRs leak into gimplifier (PR c/68513)

[Richard Biener]

On November 26, 2015 5:36:34 PM GMT+01:00, Joseph Myers <joseph@codesourcery.com> wrote:
>On Thu, 26 Nov 2015, Marek Polacek wrote:
>
>> My worry was of course C_MAYBE_CONST_EXPR_PRE.  But it seems we'll
>never have
>> any at that point, since it's already been processed and transformed
>to a
>> COMPOUND_EXPR.  But do I like this patch?  No.
>
>It's not obvious to me that it will always have been transformed - if a
>
>C_MAYBE_CONST_EXPR has escaped to gimplification, why shouldn't it be
>one 
>with C_MAYBE_CONST_EXPR_PRE?
>
>Also, on further consideration: the folding via c_fully_fold is relied 
>upon to get information about whether an expression contains anything
>that 
>cannot occur in an evaluated part of a constant expression / outside 
>sizeof in a constant expression in C90 mode.  So if a SAVE_EXPR is
>created 
>by language-independent code, c_fully_fold doesn't see inside it and
>you 
>lose that information.  What that says to me is that maybe a better 
>interim solution would be a lang hook for the folders to use to call 
>c_save_expr instead of save_expr.  And then longer term: (a) maybe any 
>folding that involves duplicating expressions and so needs to create 
>SAVE_EXPRs would better be done only at the GIMPLE level, and (b)
>folding 
>for conversions should be delayed as much as possible like other
>folding 
>(and optimizations of conversions should move from convert.c to
>match.pd).

It would be easy to simply never generate any save_exprs from genmatch generated code with the effect of disabling foldings that would need it (for correctness).

Richard.