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

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.


	Marek

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

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

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

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.

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

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

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.