diff mbox

C PATCH to kill c_save_expr or towards delayed folding for the C FE

Message ID 20170512193726.GL4910@redhat.com
State New
Headers show

Commit Message

Marek Polacek May 12, 2017, 7:37 p.m. UTC 
[ This patch depends on my previous patch that removes the fold call from
  save_expr. ]

In the effort of reducing early folding, we should avoid calling c_fully_fold
blithely, except when needed for e.g. initializers.  This is a teeny tiny step
down that path.  This patch does away with c_save_expr, which always fully
folded the expression.  Instead, I moved the SAVE_EXPR folding to c_fully_fold.
The new flag SAVE_EXPR_FOLDED_P ensures we only fold the contents once.  Also,
if the contents of the SAVE_EXPR are tree_invariant_p, we don't have to wrap
the expression with SAVE_EXPR.  I wonder if I should instead introduce a hash
map, much like C++; if we fold the contents of a SAVE_EXPR to an invariant, I
think we'll fold the expression multiple times, because in that case we cannot
check the SAVE_EXPR_FOLDED_P flag.

Comments/future directions very much appreciated.

Bootstrapped/regtested on x86_64-linux, ok for trunk once the other patch makes
it in?

2017-05-12  Marek Polacek  <polacek@redhat.com>

	* c-common.c (c_save_expr): Remove.
	(c_common_truthvalue_conversion): Remove a call to c_save_expr.
	* c-common.h (c_save_expr): Remove declaration.

	* c-convert.c (convert): Replace c_save_expr with save_expr.  Don't
	call c_fully_fold.
	* c-decl.c (grokdeclarator): Replace c_save_expr with save_expr. 
	* c-fold.c (c_fully_fold_internal): Handle SAVE_EXPR.
	* c-parser.c (c_parser_declaration_or_fndef): Replace c_save_expr with
	save_expr.
	(c_parser_conditional_expression): Likewise.
	* c-tree.h (SAVE_EXPR_FOLDED_P): Define.
	* c-typeck.c (build_modify_expr): Replace c_save_expr with save_expr.
	(process_init_element): Likewise.
	(build_binary_op): Likewise.
	(handle_omp_array_sections_1): Likewise.


	Marek

Comments

Jakub Jelinek May 12, 2017, 7:48 p.m. UTC | #1 
On Fri, May 12, 2017 at 09:37:27PM +0200, Marek Polacek wrote:
> @@ -565,6 +564,25 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands,
>  	 appropriate in any particular case.  */
>        gcc_unreachable ();
>  
> +    case SAVE_EXPR:
> +      /* Make sure to fold the contents of a SAVE_EXPR exactly once.  */
> +      if (!SAVE_EXPR_FOLDED_P (expr))
> +	{
> +	  op0 = TREE_OPERAND (expr, 0);
> +	  op0 = c_fully_fold_internal (op0, in_init, maybe_const_operands,
> +				       maybe_const_itself, for_int_const);
> +	  /* Don't wrap the folded tree in a SAVE_EXPR if we don't
> +	     have to.  */
> +	  if (tree_invariant_p (op0))
> +	    ret = op0;
> +	  else
> +	    {
> +	      TREE_OPERAND (expr, 0) = op0;
> +	      SAVE_EXPR_FOLDED_P (expr) = true;
> +	    }
> +	}

Wouldn't it be better to guard with if (!SAVE_EXPR_FOLDED_P (expr))
only c_fully_fold_internal recursion on the operand
and then use if (tree_invariant_p (op0)) unconditionally?

> @@ -113,6 +113,10 @@ along with GCC; see the file COPYING3.  If not see
>     subexpression meaning it is not a constant expression.  */
>  #define CONSTRUCTOR_NON_CONST(EXPR) TREE_LANG_FLAG_1 (CONSTRUCTOR_CHECK (EXPR))
>  
> +/* For a SAVE_EXPR, nonzero if the contents of the SAVE_EXPR have already
> +   been folded.  */

s/contents/operand/;s/have/has/ ?

Otherwise I'm all for this, but would like to give you and Joseph as C FE
maintainers the last word on this.

	Jakub
Joseph Myers May 12, 2017, 8:28 p.m. UTC | #2 
On Fri, 12 May 2017, Marek Polacek wrote:

> In the effort of reducing early folding, we should avoid calling c_fully_fold
> blithely, except when needed for e.g. initializers.  This is a teeny tiny step

Note there are several reasons for early folding in the C front end: at 
least (a) cases where logically needed (initializers and other places 
where constants are needed), (b) because warnings need a folded 
expression, (c) when the expression will go somewhere c_fully_fold does 
not recurse inside.  Also (d) convert, at least, folds regardless of 
whether it's actually necessary.

There is a case for avoiding (b) by putting the necessary information in 
the IR so the warnings can happen later from c_fully_fold, though there 
may be other possible approaches.

> @@ -146,8 +140,7 @@ convert (tree type, tree expr)
>  
>      case COMPLEX_TYPE:
>        /* If converting from COMPLEX_TYPE to a different COMPLEX_TYPE
> -	 and e is not COMPLEX_EXPR, convert_to_complex uses save_expr,
> -	 but for the C FE c_save_expr needs to be called instead.  */
> +	 and E is not COMPLEX_EXPR, convert_to_complex uses save_expr.  */
>        if (TREE_CODE (TREE_TYPE (e)) == COMPLEX_TYPE)
>  	{
>  	  if (TREE_CODE (e) != COMPLEX_EXPR)

The point of this comment is to explain why we don't just call 
convert_to_complex here (see PR 47150).  So with your changes it would 
seem appropriate to change c-convert.c back to calling convert_to_complex 
here.

The changes seem otherwise OK.
Jakub Jelinek May 13, 2017, 6:59 a.m. UTC | #3 
On Fri, May 12, 2017 at 08:28:38PM +0000, Joseph Myers wrote:
> On Fri, 12 May 2017, Marek Polacek wrote:
> 
> > In the effort of reducing early folding, we should avoid calling c_fully_fold
> > blithely, except when needed for e.g. initializers.  This is a teeny tiny step
> 
> Note there are several reasons for early folding in the C front end: at 
> least (a) cases where logically needed (initializers and other places 
> where constants are needed), (b) because warnings need a folded 
> expression, (c) when the expression will go somewhere c_fully_fold does 
> not recurse inside.  Also (d) convert, at least, folds regardless of 
> whether it's actually necessary.

The C++ FE has here a folding cache.  For (a) I think one usually just
performs that once (to fold e.g. the initializer), for (b) one might want
to fold many times, when wanting to test for some warning and then finally
when folding everything (which is where a cache can help, the first time you
fold something for a warning or when folding everything if never fold
something for a warning you cache the result and then just look it up).
(c) we should fix, (d) too, or convert to (b) kind of tests - fold something
in a test to see which path should be taken, but then actually defer
folding till later.

	Jakub
Marek Polacek May 15, 2017, 9:47 a.m. UTC | #4 
On Fri, May 12, 2017 at 09:48:28PM +0200, Jakub Jelinek wrote:
> On Fri, May 12, 2017 at 09:37:27PM +0200, Marek Polacek wrote:
> > @@ -565,6 +564,25 @@ c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands,
> >  	 appropriate in any particular case.  */
> >        gcc_unreachable ();
> >  
> > +    case SAVE_EXPR:
> > +      /* Make sure to fold the contents of a SAVE_EXPR exactly once.  */
> > +      if (!SAVE_EXPR_FOLDED_P (expr))
> > +	{
> > +	  op0 = TREE_OPERAND (expr, 0);
> > +	  op0 = c_fully_fold_internal (op0, in_init, maybe_const_operands,
> > +				       maybe_const_itself, for_int_const);
> > +	  /* Don't wrap the folded tree in a SAVE_EXPR if we don't
> > +	     have to.  */
> > +	  if (tree_invariant_p (op0))
> > +	    ret = op0;
> > +	  else
> > +	    {
> > +	      TREE_OPERAND (expr, 0) = op0;
> > +	      SAVE_EXPR_FOLDED_P (expr) = true;
> > +	    }
> > +	}
> 
> Wouldn't it be better to guard with if (!SAVE_EXPR_FOLDED_P (expr))
> only c_fully_fold_internal recursion on the operand
> and then use if (tree_invariant_p (op0)) unconditionally?
 
I don't see why that would be better.  It would mean calling tree_invariant_p
on even folded SAVE_EXPRs, and I can't see when it could be true in that case?

> > @@ -113,6 +113,10 @@ along with GCC; see the file COPYING3.  If not see
> >     subexpression meaning it is not a constant expression.  */
> >  #define CONSTRUCTOR_NON_CONST(EXPR) TREE_LANG_FLAG_1 (CONSTRUCTOR_CHECK (EXPR))
> >  
> > +/* For a SAVE_EXPR, nonzero if the contents of the SAVE_EXPR have already
> > +   been folded.  */
> 
> s/contents/operand/;s/have/has/ ?
 
Ok, if you prefer ;).

> Otherwise I'm all for this, but would like to give you and Joseph as C FE
> maintainers the last word on this.

Thanks,

	Marek
diff mbox

Patch

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index ad686d2..f606e94 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -3164,24 +3164,6 @@  c_wrap_maybe_const (tree expr, bool non_const)
   return expr;
 }
 
-/* Wrap a SAVE_EXPR around EXPR, if appropriate.  Like save_expr, but
-   for C folds the inside expression and wraps a C_MAYBE_CONST_EXPR
-   around the SAVE_EXPR if needed so that c_fully_fold does not need
-   to look inside SAVE_EXPRs.  */
-
-tree
-c_save_expr (tree expr)
-{
-  bool maybe_const = true;
-  if (c_dialect_cxx ())
-    return save_expr (expr);
-  expr = c_fully_fold (expr, false, &maybe_const);
-  expr = save_expr (expr);
-  if (!maybe_const)
-    expr = c_wrap_maybe_const (expr, true);
-  return expr;
-}
-
 /* Return whether EXPR is a declaration whose address can never be
    NULL.  */
 
@@ -3436,7 +3418,7 @@  c_common_truthvalue_conversion (location_t location, tree expr)
 
   if (TREE_CODE (TREE_TYPE (expr)) == COMPLEX_TYPE)
     {
-      tree t = (in_late_binary_op ? save_expr (expr) : c_save_expr (expr));
+      tree t = save_expr (expr);
       expr = (build_binary_op
 	      (EXPR_LOCATION (expr),
 	       (TREE_SIDE_EFFECTS (expr)
diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index 9e3982d..3981544 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -836,7 +836,6 @@  extern enum conversion_safety unsafe_conversion_p (location_t, tree, tree,
 extern bool decl_with_nonnull_addr_p (const_tree);
 extern tree c_fully_fold (tree, bool, bool *);
 extern tree c_wrap_maybe_const (tree, bool);
-extern tree c_save_expr (tree);
 extern tree c_common_truthvalue_conversion (location_t, tree);
 extern void c_apply_type_quals_to_decl (int, tree);
 extern tree c_sizeof_or_alignof_type (location_t, tree, bool, bool, int);
diff --git gcc/c/c-convert.c gcc/c/c-convert.c
index 163feff..a7764f0 100644
--- gcc/c/c-convert.c
+++ gcc/c/c-convert.c
@@ -111,13 +111,7 @@  convert (tree type, tree expr)
 	  && COMPLETE_TYPE_P (type)
 	  && do_ubsan_in_current_function ())
 	{
-	  if (in_late_binary_op)
-	    expr = save_expr (expr);
-	  else
-	    {
-	      expr = c_save_expr (expr);
-	      expr = c_fully_fold (expr, false, NULL);
-	    }
+	  expr = save_expr (expr);
 	  tree check = ubsan_instrument_float_cast (loc, type, expr);
 	  expr = fold_build1 (FIX_TRUNC_EXPR, type, expr);
 	  if (check == NULL_TREE)
@@ -146,8 +140,7 @@  convert (tree type, tree expr)
 
     case COMPLEX_TYPE:
       /* If converting from COMPLEX_TYPE to a different COMPLEX_TYPE
-	 and e is not COMPLEX_EXPR, convert_to_complex uses save_expr,
-	 but for the C FE c_save_expr needs to be called instead.  */
+	 and E is not COMPLEX_EXPR, convert_to_complex uses save_expr.  */
       if (TREE_CODE (TREE_TYPE (e)) == COMPLEX_TYPE)
 	{
 	  if (TREE_CODE (e) != COMPLEX_EXPR)
@@ -155,10 +148,7 @@  convert (tree type, tree expr)
 	      tree subtype = TREE_TYPE (type);
 	      tree elt_type = TREE_TYPE (TREE_TYPE (e));
 
-	      if (in_late_binary_op)
-		e = save_expr (e);
-	      else
-		e = c_save_expr (e);
+	      e = save_expr (e);
 	      ret
 		= fold_build2_loc (loc, COMPLEX_EXPR, type,
 				   convert (subtype,
diff --git gcc/c/c-decl.c gcc/c/c-decl.c
index b779d37..8ae09c4 100644
--- gcc/c/c-decl.c
+++ gcc/c/c-decl.c
@@ -6076,7 +6076,7 @@  grokdeclarator (const struct c_declarator *declarator,
 			&& do_ubsan_in_current_function ())
 		      {
 			/* Evaluate the array size only once.  */
-			size = c_save_expr (size);
+			size = save_expr (size);
 			size = c_fully_fold (size, false, NULL);
 		        size = fold_build2 (COMPOUND_EXPR, TREE_TYPE (size),
 					    ubsan_instrument_vla (loc, size),
diff --git gcc/c/c-fold.c gcc/c/c-fold.c
index b060d76..1baee44 100644
--- gcc/c/c-fold.c
+++ gcc/c/c-fold.c
@@ -120,12 +120,11 @@  c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands,
   bool unused_p;
   source_range old_range;
 
-  /* Constants, declarations, statements, errors, SAVE_EXPRs and
-     anything else not counted as an expression cannot usefully be
-     folded further at this point.  */
+  /* Constants, declarations, statements, errors, and anything else not
+     counted as an expression cannot usefully be folded further at this
+     point.  */
   if (!IS_EXPR_CODE_CLASS (kind)
-      || kind == tcc_statement
-      || code == SAVE_EXPR)
+      || kind == tcc_statement)
     return expr;
 
   if (IS_EXPR_CODE_CLASS (kind))
@@ -565,6 +564,25 @@  c_fully_fold_internal (tree expr, bool in_init, bool *maybe_const_operands,
 	 appropriate in any particular case.  */
       gcc_unreachable ();
 
+    case SAVE_EXPR:
+      /* Make sure to fold the contents of a SAVE_EXPR exactly once.  */
+      if (!SAVE_EXPR_FOLDED_P (expr))
+	{
+	  op0 = TREE_OPERAND (expr, 0);
+	  op0 = c_fully_fold_internal (op0, in_init, maybe_const_operands,
+				       maybe_const_itself, for_int_const);
+	  /* Don't wrap the folded tree in a SAVE_EXPR if we don't
+	     have to.  */
+	  if (tree_invariant_p (op0))
+	    ret = op0;
+	  else
+	    {
+	      TREE_OPERAND (expr, 0) = op0;
+	      SAVE_EXPR_FOLDED_P (expr) = true;
+	    }
+	}
+      goto out;
+
     default:
       /* Various codes may appear through folding built-in functions
 	 and their arguments.  */
diff --git gcc/c/c-parser.c gcc/c/c-parser.c
index 90d2d17..96c0749 100644
--- gcc/c/c-parser.c
+++ gcc/c/c-parser.c
@@ -1880,7 +1880,7 @@  c_parser_declaration_or_fndef (c_parser *parser, bool fndef_ok,
 		  bool vm_type = variably_modified_type_p (init_type,
 							   NULL_TREE);
 		  if (vm_type)
-		    init.value = c_save_expr (init.value);
+		    init.value = save_expr (init.value);
 		  finish_init ();
 		  specs->typespec_kind = ctsk_typeof;
 		  specs->locations[cdw_typedef] = init_loc;
@@ -6500,7 +6500,7 @@  c_parser_conditional_expression (c_parser *parser, struct c_expr *after,
 	e = TREE_OPERAND (e, 1);
       warn_for_omitted_condop (middle_loc, e);
       /* Make sure first operand is calculated only once.  */
-      exp1.value = c_save_expr (default_conversion (cond.value));
+      exp1.value = save_expr (default_conversion (cond.value));
       if (eptype)
 	exp1.value = build1 (EXCESS_PRECISION_EXPR, eptype, exp1.value);
       exp1.original_type = NULL;
diff --git gcc/c/c-tree.h gcc/c/c-tree.h
index 8d232a4..a27a2c7 100644
--- gcc/c/c-tree.h
+++ gcc/c/c-tree.h
@@ -113,6 +113,10 @@  along with GCC; see the file COPYING3.  If not see
    subexpression meaning it is not a constant expression.  */
 #define CONSTRUCTOR_NON_CONST(EXPR) TREE_LANG_FLAG_1 (CONSTRUCTOR_CHECK (EXPR))
 
+/* For a SAVE_EXPR, nonzero if the contents of the SAVE_EXPR have already
+   been folded.  */
+#define SAVE_EXPR_FOLDED_P(EXP)	TREE_LANG_FLAG_1 (SAVE_EXPR_CHECK (EXP))
+
 /* Record parser information about an expression that is irrelevant
    for code generation alongside a tree representing its value.  */
 struct c_expr
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 6f9909c..1edf521 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -5777,7 +5777,7 @@  build_modify_expr (location_t location, tree lhs, tree lhs_origtype,
 	     that modify LHS.  */
 	  if (TREE_SIDE_EFFECTS (rhs))
 	    {
-	      newrhs = in_late_binary_op ? save_expr (rhs) : c_save_expr (rhs);
+	      newrhs = save_expr (rhs);
 	      rhseval = newrhs;
 	    }
 	  newrhs = build_binary_op (location,
@@ -9301,7 +9301,7 @@  process_init_element (location_t loc, struct c_expr value, bool implicit,
 	      semantic_type = TREE_TYPE (value.value);
 	      value.value = TREE_OPERAND (value.value, 0);
 	    }
-	  value.value = c_save_expr (value.value);
+	  value.value = save_expr (value.value);
 	  if (semantic_type)
 	    value.value = build1 (EXCESS_PRECISION_EXPR, semantic_type,
 				  value.value);
@@ -11621,7 +11621,7 @@  build_binary_op (location_t location, enum tree_code code,
 	    return error_mark_node;
 	  if (first_complex)
 	    {
-	      op0 = c_save_expr (op0);
+	      op0 = save_expr (op0);
 	      real = build_unary_op (EXPR_LOCATION (orig_op0), REALPART_EXPR,
 				     op0, true);
 	      imag = build_unary_op (EXPR_LOCATION (orig_op0), IMAGPART_EXPR,
@@ -11630,7 +11630,7 @@  build_binary_op (location_t location, enum tree_code code,
 		{
 		case MULT_EXPR:
 		case TRUNC_DIV_EXPR:
-		  op1 = c_save_expr (op1);
+		  op1 = save_expr (op1);
 		  imag = build2 (resultcode, real_type, imag, op1);
 		  /* Fall through.  */
 		case PLUS_EXPR:
@@ -11643,7 +11643,7 @@  build_binary_op (location_t location, enum tree_code code,
 	    }
 	  else
 	    {
-	      op1 = c_save_expr (op1);
+	      op1 = save_expr (op1);
 	      real = build_unary_op (EXPR_LOCATION (orig_op1), REALPART_EXPR,
 				     op1, true);
 	      imag = build_unary_op (EXPR_LOCATION (orig_op1), IMAGPART_EXPR,
@@ -11651,7 +11651,7 @@  build_binary_op (location_t location, enum tree_code code,
 	      switch (code)
 		{
 		case MULT_EXPR:
-		  op0 = c_save_expr (op0);
+		  op0 = save_expr (op0);
 		  imag = build2 (resultcode, real_type, op0, imag);
 		  /* Fall through.  */
 		case PLUS_EXPR:
@@ -11835,8 +11835,8 @@  build_binary_op (location_t location, enum tree_code code,
       && !require_constant_value)
     {
       /* OP0 and/or OP1 might have side-effects.  */
-      op0 = c_save_expr (op0);
-      op1 = c_save_expr (op1);
+      op0 = save_expr (op0);
+      op1 = save_expr (op1);
       op0 = c_fully_fold (op0, false, NULL);
       op1 = c_fully_fold (op1, false, NULL);
       if (doing_div_or_mod && (flag_sanitize & (SANITIZE_DIVIDE
@@ -12435,7 +12435,7 @@  handle_omp_array_sections_1 (tree c, tree t, vec<tree> &types,
       /* For [lb:] we will need to evaluate lb more than once.  */
       if (length == NULL_TREE && OMP_CLAUSE_CODE (c) != OMP_CLAUSE_DEPEND)
 	{
-	  tree lb = c_save_expr (low_bound);
+	  tree lb = save_expr (low_bound);
 	  if (lb != low_bound)
 	    {
 	      TREE_PURPOSE (t) = lb;
@@ -12480,7 +12480,7 @@  handle_omp_array_sections_1 (tree c, tree t, vec<tree> &types,
   if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_DEPEND)
     types.safe_push (TREE_TYPE (ret));
   /* We will need to evaluate lb more than once.  */
-  tree lb = c_save_expr (low_bound);
+  tree lb = save_expr (low_bound);
   if (lb != low_bound)
     {
       TREE_PURPOSE (t) = lb;