[PATCH] Add save_expr langhook (PR c/68513)

[PATCH] Add save_expr langhook (PR c/68513)

[Marek Polacek]

As suggested here <https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03271.html>
and here <https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03282.html>, this patch
adds a new langhook to distinguish whether to call c_save_expr or save_expr
from match.pd.  Does this look reasonable?

I didn't know where to put setting of in_late_processing.  With the current
placement, we won't (for valid programs) call c_save_expr from c_genericize
or c_gimplify_expr.

I suppose I should also modify save_expr in fold-const.c to call it via the
langhook, if this approach is sane.  Dunno.

Bootstrapped/regtested on x86_64-linux.

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

	PR c/68513
	* c-common.c (in_late_processing): New global.
	(c_common_save_expr): New function.
	* c-common.h (in_late_processing, c_common_save_expr): Declare.

	* c-objc-common.h (LANG_HOOKS_SAVE_EXPR): Define.
	* c-parser.c (c_parser_compound_statement): Set IN_LATE_PROCESSING.

	* generic-match-head.c: Include "langhooks.h".
	* genmatch.c (dt_simplify::gen_1): Call save_expr via langhook.
	* langhooks-def.h (LANG_HOOKS_SAVE_EXPR): Define.
	* langhooks.h (struct lang_hooks): Add save_expr langhook.

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

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index fe0a235..850bee9 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -271,6 +271,12 @@ int c_inhibit_evaluation_warnings;
    be generated.  */
 bool in_late_binary_op;
 
+/* When true, all the constant expression checks from parsing should have been
+   done.  This is used so that fold knows whether to call c_save_expr (thus
+   c_fully_fold is called on the expression), or whether to call save_expr via
+   c_common_save_expr langhook.  */
+bool in_late_processing;
+
 /* Whether lexing has been completed, so subsequent preprocessor
    errors should use the compiler's input_location.  */
 bool done_lexing = false;
@@ -4928,6 +4934,15 @@ c_save_expr (tree expr)
   return expr;
 }
 
+/* The C version of the save_expr langhook.  Either call save_expr or c_save_expr,
+   depending on IN_LATE_PROCESSING.  */
+
+tree
+c_common_save_expr (tree expr)
+{
+  return in_late_processing ? save_expr (expr) : c_save_expr (expr);
+}
+
 /* Return whether EXPR is a declaration whose address can never be
    NULL.  */
 
diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index bad8d05..e2d4ba9 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -771,6 +771,7 @@ extern void c_register_addr_space (const char *str, addr_space_t as);
 
 /* In c-common.c.  */
 extern bool in_late_binary_op;
+extern bool in_late_processing;
 extern const char *c_addr_space_name (addr_space_t as);
 extern tree identifier_global_value (tree);
 extern tree c_linkage_bindings (tree);
@@ -812,6 +813,7 @@ extern tree c_fully_fold (tree, bool, bool *);
 extern tree decl_constant_value_for_optimization (tree);
 extern tree c_wrap_maybe_const (tree, bool);
 extern tree c_save_expr (tree);
+extern tree c_common_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-objc-common.h gcc/c/c-objc-common.h
index 50c9f54..9fd3722 100644
--- gcc/c/c-objc-common.h
+++ gcc/c/c-objc-common.h
@@ -60,6 +60,8 @@ along with GCC; see the file COPYING3.  If not see
 #define LANG_HOOKS_BUILTIN_FUNCTION c_builtin_function
 #undef  LANG_HOOKS_BUILTIN_FUNCTION_EXT_SCOPE
 #define LANG_HOOKS_BUILTIN_FUNCTION_EXT_SCOPE c_builtin_function_ext_scope
+#undef LANG_HOOKS_SAVE_EXPR
+#define LANG_HOOKS_SAVE_EXPR c_common_save_expr
 
 /* Attribute hooks.  */
 #undef LANG_HOOKS_COMMON_ATTRIBUTE_TABLE
diff --git gcc/c/c-parser.c gcc/c/c-parser.c
index 0259f66..3f7c458 100644
--- gcc/c/c-parser.c
+++ gcc/c/c-parser.c
@@ -4586,6 +4586,9 @@ c_parser_compound_statement (c_parser *parser)
 {
   tree stmt;
   location_t brace_loc;
+
+  in_late_processing = false;
+
   brace_loc = c_parser_peek_token (parser)->location;
   if (!c_parser_require (parser, CPP_OPEN_BRACE, "expected %<{%>"))
     {
@@ -4598,6 +4601,9 @@ c_parser_compound_statement (c_parser *parser)
   stmt = c_begin_compound_stmt (true);
   c_parser_compound_statement_nostart (parser);
 
+  /* From now on, the fold machinery shouldn't call c_save_expr.  */
+  in_late_processing = true;
+
   /* If the compound stmt contains array notations, then we expand them.  */
   if (flag_cilkplus && contains_array_notation_expr (stmt))
     stmt = expand_array_notation_exprs (stmt);
diff --git gcc/generic-match-head.c gcc/generic-match-head.c
index f55f91e..8fc9d40 100644
--- gcc/generic-match-head.c
+++ gcc/generic-match-head.c
@@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "builtins.h"
 #include "dumpfile.h"
 #include "case-cfn-macros.h"
+#include "langhooks.h"
 
 
 /* Routine to determine if the types T1 and T2 are effectively
diff --git gcc/genmatch.c gcc/genmatch.c
index 67d1c66..c73a94d 100644
--- gcc/genmatch.c
+++ gcc/genmatch.c
@@ -3114,7 +3114,8 @@ dt_simplify::gen_1 (FILE *f, int indent, bool gimple, operand *result)
 		  continue;
 		if (cinfo.info[i].result_use_count > 1)
 		  fprintf_indent (f, indent,
-				  "captures[%d] = save_expr (captures[%d]);\n",
+				  "captures[%d] = lang_hooks.save_expr "
+				  "(captures[%d]);\n",
 				  i, i);
 	      }
 	  for (unsigned j = 0; j < e->ops.length (); ++j)
diff --git gcc/langhooks-def.h gcc/langhooks-def.h
index 18ac84d..6f5babe 100644
--- gcc/langhooks-def.h
+++ gcc/langhooks-def.h
@@ -120,6 +120,7 @@ extern bool lhd_omp_mappable_type (tree);
 #define LANG_HOOKS_BLOCK_MAY_FALLTHRU	hook_bool_const_tree_true
 #define LANG_HOOKS_EH_USE_CXA_END_CLEANUP	false
 #define LANG_HOOKS_DEEP_UNSHARING	false
+#define LANG_HOOKS_SAVE_EXPR		save_expr
 
 /* Attribute hooks.  */
 #define LANG_HOOKS_ATTRIBUTE_TABLE		NULL
@@ -313,7 +314,8 @@ extern void lhd_end_section (void);
   LANG_HOOKS_EH_PROTECT_CLEANUP_ACTIONS, \
   LANG_HOOKS_BLOCK_MAY_FALLTHRU, \
   LANG_HOOKS_EH_USE_CXA_END_CLEANUP, \
-  LANG_HOOKS_DEEP_UNSHARING \
+  LANG_HOOKS_DEEP_UNSHARING, \
+  LANG_HOOKS_SAVE_EXPR \
 }
 
 #endif /* GCC_LANG_HOOKS_DEF_H */
diff --git gcc/langhooks.h gcc/langhooks.h
index d8d01fa..18f332d 100644
--- gcc/langhooks.h
+++ gcc/langhooks.h
@@ -488,6 +488,9 @@ struct lang_hooks
      gimplification.  */
   bool deep_unsharing;
 
+  /* Used by the fold machinery to either call save_expr or c_save_expr.  */
+  tree (*save_expr) (tree);
+
   /* Whenever you add entries here, make sure you adjust langhooks-def.h
      and langhooks.c accordingly.  */
 };
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] Add save_expr langhook (PR c/68513)

[Marek Polacek]

On Fri, Nov 27, 2015 at 07:55:43PM +0100, Marek Polacek wrote:
> +/* The C version of the save_expr langhook.  Either call save_expr or c_save_expr,
> +   depending on IN_LATE_PROCESSING.  */

Consider this too long line fixed.

	Marek

Re: [PATCH] Add save_expr langhook (PR c/68513)

[Joseph Myers]

On Fri, 27 Nov 2015, Marek Polacek wrote:

> I didn't know where to put setting of in_late_processing.  With the current
> placement, we won't (for valid programs) call c_save_expr from c_genericize
> or c_gimplify_expr.

Well, the placement in this patch (in c_parser_compound_statement) is 
certainly wrong.  It doesn't even save and restore, so after one compound 
statement inside another, parsing would continue with in_late_processing 
wrongly set.  And c_save_expr is logically right for any parsing outside 
compound statements as well (arbitrary expressions can occur in sizeof 
outside functions and in VLA parameter sizes and should follow the normal 
rules for what's a constant expression - there's a known bug that 
statement expressions are wrongly rejected in such contexts).

Starting from first principles: parsing takes place from within 
c_parse_file as the sole external entry point to the parser.  So you could 
have a parsing_input variable that starts off as false, and where 
c_parse_file saves it, sets to true, and restores the saved value at the 
end.  Then you'd use c_save_expr if parsing_input && !in_late_binary_op.

If that doesn't work, it means there are cases where the hook gets called 
from folding that takes place during parsing, on expressions that will not 
subsequently go through c_fully_fold, but without in_late_binary_op set.  
Knowing what those cases are might help work out any fix for them that is 
needed.

> I suppose I should also modify save_expr in fold-const.c to call it via the
> langhook, if this approach is sane.  Dunno.

That's a complication.  When the folding is taking place from within 
c_fully_fold (and so the sub-expressions have already been folded, and had 
their C_MAYBE_CONST_EXPRs removed, and the result of folding will not be 
re-folded), it should be using save_expr not c_save_expr.  So maybe the 
hook needs to say: use c_save_expr, if parsing, not in_late_binary_op and 
not folding from within c_fully_fold.

Again long term we should aim for the representation during parsing not to 
need SAVE_EXPRs and for the folding that creates them (and the other 
folding for optimization in general) to happen only after parsing....

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add save_expr langhook (PR c/68513)

[Richard Biener]

On November 27, 2015 7:55:43 PM GMT+01:00, Marek Polacek <polacek@redhat.com> wrote:
>As suggested here
><https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03271.html>
>and here <https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03282.html>,
>this patch
>adds a new langhook to distinguish whether to call c_save_expr or
>save_expr
>from match.pd.  Does this look reasonable?
>
>I didn't know where to put setting of in_late_processing.  With the
>current
>placement, we won't (for valid programs) call c_save_expr from
>c_genericize
>or c_gimplify_expr.
>
>I suppose I should also modify save_expr in fold-const.c to call it via
>the
>langhook, if this approach is sane.  Dunno.

I don't like this at all.

Different approach: after the FE folds (unexpectedly?), scan the result for SAVE_EXPRs and if found, drop the folding.

Richard.

>Bootstrapped/regtested on x86_64-linux.
>
>2015-11-27  Marek Polacek  <polacek@redhat.com>
>
>	PR c/68513
>	* c-common.c (in_late_processing): New global.
>	(c_common_save_expr): New function.
>	* c-common.h (in_late_processing, c_common_save_expr): Declare.
>
>	* c-objc-common.h (LANG_HOOKS_SAVE_EXPR): Define.
>	* c-parser.c (c_parser_compound_statement): Set IN_LATE_PROCESSING.
>
>	* generic-match-head.c: Include "langhooks.h".
>	* genmatch.c (dt_simplify::gen_1): Call save_expr via langhook.
>	* langhooks-def.h (LANG_HOOKS_SAVE_EXPR): Define.
>	* langhooks.h (struct lang_hooks): Add save_expr langhook.
>
>	* gcc.dg/torture/pr68513.c: New test.
>
>diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
>index fe0a235..850bee9 100644
>--- gcc/c-family/c-common.c
>+++ gcc/c-family/c-common.c
>@@ -271,6 +271,12 @@ int c_inhibit_evaluation_warnings;
>    be generated.  */
> bool in_late_binary_op;
> 
>+/* When true, all the constant expression checks from parsing should
>have been
>+   done.  This is used so that fold knows whether to call c_save_expr
>(thus
>+   c_fully_fold is called on the expression), or whether to call
>save_expr via
>+   c_common_save_expr langhook.  */
>+bool in_late_processing;
>+
> /* Whether lexing has been completed, so subsequent preprocessor
>    errors should use the compiler's input_location.  */
> bool done_lexing = false;
>@@ -4928,6 +4934,15 @@ c_save_expr (tree expr)
>   return expr;
> }
> 
>+/* The C version of the save_expr langhook.  Either call save_expr or
>c_save_expr,
>+   depending on IN_LATE_PROCESSING.  */
>+
>+tree
>+c_common_save_expr (tree expr)
>+{
>+  return in_late_processing ? save_expr (expr) : c_save_expr (expr);
>+}
>+
> /* Return whether EXPR is a declaration whose address can never be
>    NULL.  */
> 
>diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
>index bad8d05..e2d4ba9 100644
>--- gcc/c-family/c-common.h
>+++ gcc/c-family/c-common.h
>@@ -771,6 +771,7 @@ extern void c_register_addr_space (const char *str,
>addr_space_t as);
> 
> /* In c-common.c.  */
> extern bool in_late_binary_op;
>+extern bool in_late_processing;
> extern const char *c_addr_space_name (addr_space_t as);
> extern tree identifier_global_value (tree);
> extern tree c_linkage_bindings (tree);
>@@ -812,6 +813,7 @@ extern tree c_fully_fold (tree, bool, bool *);
> extern tree decl_constant_value_for_optimization (tree);
> extern tree c_wrap_maybe_const (tree, bool);
> extern tree c_save_expr (tree);
>+extern tree c_common_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-objc-common.h gcc/c/c-objc-common.h
>index 50c9f54..9fd3722 100644
>--- gcc/c/c-objc-common.h
>+++ gcc/c/c-objc-common.h
>@@ -60,6 +60,8 @@ along with GCC; see the file COPYING3.  If not see
> #define LANG_HOOKS_BUILTIN_FUNCTION c_builtin_function
> #undef  LANG_HOOKS_BUILTIN_FUNCTION_EXT_SCOPE
>#define LANG_HOOKS_BUILTIN_FUNCTION_EXT_SCOPE
>c_builtin_function_ext_scope
>+#undef LANG_HOOKS_SAVE_EXPR
>+#define LANG_HOOKS_SAVE_EXPR c_common_save_expr
> 
> /* Attribute hooks.  */
> #undef LANG_HOOKS_COMMON_ATTRIBUTE_TABLE
>diff --git gcc/c/c-parser.c gcc/c/c-parser.c
>index 0259f66..3f7c458 100644
>--- gcc/c/c-parser.c
>+++ gcc/c/c-parser.c
>@@ -4586,6 +4586,9 @@ c_parser_compound_statement (c_parser *parser)
> {
>   tree stmt;
>   location_t brace_loc;
>+
>+  in_late_processing = false;
>+
>   brace_loc = c_parser_peek_token (parser)->location;
>   if (!c_parser_require (parser, CPP_OPEN_BRACE, "expected %<{%>"))
>     {
>@@ -4598,6 +4601,9 @@ c_parser_compound_statement (c_parser *parser)
>   stmt = c_begin_compound_stmt (true);
>   c_parser_compound_statement_nostart (parser);
> 
>+  /* From now on, the fold machinery shouldn't call c_save_expr.  */
>+  in_late_processing = true;
>+
>/* If the compound stmt contains array notations, then we expand them. 
>*/
>   if (flag_cilkplus && contains_array_notation_expr (stmt))
>     stmt = expand_array_notation_exprs (stmt);
>diff --git gcc/generic-match-head.c gcc/generic-match-head.c
>index f55f91e..8fc9d40 100644
>--- gcc/generic-match-head.c
>+++ gcc/generic-match-head.c
>@@ -33,6 +33,7 @@ along with GCC; see the file COPYING3.  If not see
> #include "builtins.h"
> #include "dumpfile.h"
> #include "case-cfn-macros.h"
>+#include "langhooks.h"
> 
> 
> /* Routine to determine if the types T1 and T2 are effectively
>diff --git gcc/genmatch.c gcc/genmatch.c
>index 67d1c66..c73a94d 100644
>--- gcc/genmatch.c
>+++ gcc/genmatch.c
>@@ -3114,7 +3114,8 @@ dt_simplify::gen_1 (FILE *f, int indent, bool
>gimple, operand *result)
> 		  continue;
> 		if (cinfo.info[i].result_use_count > 1)
> 		  fprintf_indent (f, indent,
>-				  "captures[%d] = save_expr (captures[%d]);\n",
>+				  "captures[%d] = lang_hooks.save_expr "
>+				  "(captures[%d]);\n",
> 				  i, i);
> 	      }
> 	  for (unsigned j = 0; j < e->ops.length (); ++j)
>diff --git gcc/langhooks-def.h gcc/langhooks-def.h
>index 18ac84d..6f5babe 100644
>--- gcc/langhooks-def.h
>+++ gcc/langhooks-def.h
>@@ -120,6 +120,7 @@ extern bool lhd_omp_mappable_type (tree);
> #define LANG_HOOKS_BLOCK_MAY_FALLTHRU	hook_bool_const_tree_true
> #define LANG_HOOKS_EH_USE_CXA_END_CLEANUP	false
> #define LANG_HOOKS_DEEP_UNSHARING	false
>+#define LANG_HOOKS_SAVE_EXPR		save_expr
> 
> /* Attribute hooks.  */
> #define LANG_HOOKS_ATTRIBUTE_TABLE		NULL
>@@ -313,7 +314,8 @@ extern void lhd_end_section (void);
>   LANG_HOOKS_EH_PROTECT_CLEANUP_ACTIONS, \
>   LANG_HOOKS_BLOCK_MAY_FALLTHRU, \
>   LANG_HOOKS_EH_USE_CXA_END_CLEANUP, \
>-  LANG_HOOKS_DEEP_UNSHARING \
>+  LANG_HOOKS_DEEP_UNSHARING, \
>+  LANG_HOOKS_SAVE_EXPR \
> }
> 
> #endif /* GCC_LANG_HOOKS_DEF_H */
>diff --git gcc/langhooks.h gcc/langhooks.h
>index d8d01fa..18f332d 100644
>--- gcc/langhooks.h
>+++ gcc/langhooks.h
>@@ -488,6 +488,9 @@ struct lang_hooks
>      gimplification.  */
>   bool deep_unsharing;
> 
>+  /* Used by the fold machinery to either call save_expr or
>c_save_expr.  */
>+  tree (*save_expr) (tree);
>+
> /* Whenever you add entries here, make sure you adjust langhooks-def.h
>      and langhooks.c accordingly.  */
> };
>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] Add save_expr langhook (PR c/68513)

[Joseph Myers]

On Sat, 28 Nov 2015, Richard Biener wrote:

> Different approach: after the FE folds (unexpectedly?), scan the result 
> for SAVE_EXPRs and if found, drop the folding.

Or, if conversions are going to fold from language-independent code (which 
is the underlying problem here - a conversion without folding would be 
preferred once the fallout from that can be resolved), make the front end 
fold with c_fully_fold before doing the conversion, and wrap the result of 
the conversion in a C_MAYBE_CONST_EXPR with c_wrap_maybe_const in the same 
way as done in other places that fold early (if either c_fully_fold 
indicates it can't occur in a constant expression, or the result of 
folding / conversion is not an INTEGER_CST).

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH] Add save_expr langhook (PR c/68513)

[Marek Polacek]

On Fri, Nov 27, 2015 at 10:43:42PM +0000, Joseph Myers wrote:
> On Fri, 27 Nov 2015, Marek Polacek wrote:
> 
> > I didn't know where to put setting of in_late_processing.  With the current
> > placement, we won't (for valid programs) call c_save_expr from c_genericize
> > or c_gimplify_expr.
> 
> Well, the placement in this patch (in c_parser_compound_statement) is 
> certainly wrong.  It doesn't even save and restore, so after one compound 
> statement inside another, parsing would continue with in_late_processing 
> wrongly set.  And c_save_expr is logically right for any parsing outside 
> compound statements as well (arbitrary expressions can occur in sizeof 
> outside functions and in VLA parameter sizes and should follow the normal 
> rules for what's a constant expression - there's a known bug that 
> statement expressions are wrongly rejected in such contexts).
 
Indeed.  I don't know what I was thinking. :/

> Starting from first principles: parsing takes place from within 
> c_parse_file as the sole external entry point to the parser.  So you could 
> have a parsing_input variable that starts off as false, and where 
> c_parse_file saves it, sets to true, and restores the saved value at the 
> end.  Then you'd use c_save_expr if parsing_input && !in_late_binary_op.
> 
> If that doesn't work, it means there are cases where the hook gets called 
> from folding that takes place during parsing, on expressions that will not 
> subsequently go through c_fully_fold, but without in_late_binary_op set.  
> Knowing what those cases are might help work out any fix for them that is 
> needed.

I'm not sanguine about doing this reliably in stage3.  I think I'll try the
other approach mentioned later in this thread.
 
> > I suppose I should also modify save_expr in fold-const.c to call it via the
> > langhook, if this approach is sane.  Dunno.
> 
> That's a complication.  When the folding is taking place from within 
> c_fully_fold (and so the sub-expressions have already been folded, and had 
> their C_MAYBE_CONST_EXPRs removed, and the result of folding will not be 
> re-folded), it should be using save_expr not c_save_expr.  So maybe the 
> hook needs to say: use c_save_expr, if parsing, not in_late_binary_op and 
> not folding from within c_fully_fold.
 
Oh, I see :(.

> Again long term we should aim for the representation during parsing not to 
> need SAVE_EXPRs and for the folding that creates them (and the other 
> folding for optimization in general) to happen only after parsing....

Yeah, let's strike that for gcc7.

Thanks,

	Marek

Re: [PATCH] Add save_expr langhook (PR c/68513)

[Marek Polacek]

On Sat, Nov 28, 2015 at 04:05:30PM +0000, Joseph Myers wrote:
> On Sat, 28 Nov 2015, Richard Biener wrote:
> 
> > Different approach: after the FE folds (unexpectedly?), scan the result 
> > for SAVE_EXPRs and if found, drop the folding.
> 
> Or, if conversions are going to fold from language-independent code (which 
> is the underlying problem here - a conversion without folding would be 
> preferred once the fallout from that can be resolved), make the front end 
> fold with c_fully_fold before doing the conversion, and wrap the result of 
> the conversion in a C_MAYBE_CONST_EXPR with c_wrap_maybe_const in the same 
> way as done in other places that fold early (if either c_fully_fold 
> indicates it can't occur in a constant expression, or the result of 
> folding / conversion is not an INTEGER_CST).

Unfortunately, even this doesn't seem to work :(; I'm getting leaked
C_MAYBE_CONST_EXPRs e.g. when converting to (_Complex float), and a bunch of
missing warnings resulting in big testsuite fallout.

	Marek

Re: [PATCH] Add save_expr langhook (PR c/68513)

[Marek Polacek]

On Sat, Nov 28, 2015 at 08:50:12AM +0100, Richard Biener wrote:
> Different approach: after the FE folds (unexpectedly?), scan the result for
> SAVE_EXPRs and if found, drop the folding.

Neither this fixes this problem completely, because we simply don't know where
those SAVE_EXPRs might be introduced: it might be convert(), but e.g. when I
changed the original testcase a tiny bit (added -), then those SAVE_EXPRs were
introduced in a different spot (via c_process_stmt_expr -> c_fully_fold).

	Marek

Re: [PATCH] Add save_expr langhook (PR c/68513)

[Richard Biener]

On Mon, 30 Nov 2015, Marek Polacek wrote:

> On Sat, Nov 28, 2015 at 08:50:12AM +0100, Richard Biener wrote:
> > Different approach: after the FE folds (unexpectedly?), scan the result for
> > SAVE_EXPRs and if found, drop the folding.
> 
> Neither this fixes this problem completely, because we simply don't know where
> those SAVE_EXPRs might be introduced: it might be convert(), but e.g. when I
> changed the original testcase a tiny bit (added -), then those SAVE_EXPRs were
> introduced in a different spot (via c_process_stmt_expr -> c_fully_fold).

So the following "disables" save_expr generation from generic-match.c
by failing to simplify if save_expr would end up not returning a
non-save_expr.

I expect this will make fixing PR68590 difficult (w/o re-introducing
some fold-const.c code or changing genmatch to "special-case"
things).

The other option for this PR is to re-introduce the TREE_SIDE_EFFECTS
check I removed earlier (to avoid un-CSEing large expressions at
-O0 for example) and thus only FAIL if the save_expr were needed
for correctness.

Richard.

Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 231065)
+++ gcc/tree.c	(working copy)
@@ -3231,8 +3231,6 @@ decl_address_ip_invariant_p (const_tree
    not handle arithmetic; that's handled in skip_simple_arithmetic and
    tree_invariant_p).  */
 
-static bool tree_invariant_p (tree t);
-
 static bool
 tree_invariant_p_1 (tree t)
 {
@@ -3282,7 +3280,7 @@ tree_invariant_p_1 (tree t)
 
 /* Return true if T is function-invariant.  */
 
-static bool
+bool
 tree_invariant_p (tree t)
 {
   tree inner = skip_simple_arithmetic (t);
Index: gcc/tree.h
===================================================================
--- gcc/tree.h	(revision 231065)
+++ gcc/tree.h	(working copy)
@@ -4320,6 +4320,10 @@ extern tree staticp (tree);
 
 extern tree save_expr (tree);
 
+/* Return true if T is function-invariant.  */
+
+extern bool tree_invariant_p (tree);
+
 /* Look inside EXPR into any simple arithmetic operations.  Return the
    outermost non-arithmetic or non-invariant node.  */
 
Index: gcc/genmatch.c
===================================================================
--- gcc/genmatch.c	(revision 231065)
+++ gcc/genmatch.c	(working copy)
@@ -3106,7 +3106,9 @@ dt_simplify::gen_1 (FILE *f, int indent,
 	  else if (is_a <predicate_id *> (opr))
 	    is_predicate = true;
 	  /* Search for captures used multiple times in the result expression
-	     and dependent on TREE_SIDE_EFFECTS emit a SAVE_EXPR.  */
+	     and check if we can safely evaluate it multiple times.  Otherwise
+	     fail, avoiding a SAVE_EXPR because that confuses the C FE
+	     const expression folding.  */
 	  if (!is_predicate)
 	    for (int i = 0; i < s->capture_max + 1; ++i)
 	      {
@@ -3114,8 +3116,8 @@ dt_simplify::gen_1 (FILE *f, int indent,
 		  continue;
 		if (cinfo.info[i].result_use_count > 1)
 		  fprintf_indent (f, indent,
-				  "captures[%d] = save_expr (captures[%d]);\n",
-				  i, i);
+				  "if (! tree_invariant_p (captures[%d])) "
+				  "return NULL_TREE;\n", i);
 	      }
 	  for (unsigned j = 0; j < e->ops.length (); ++j)
 	    {

Re: [PATCH] Add save_expr langhook (PR c/68513)

[Richard Biener]

On Mon, 30 Nov 2015, Richard Biener wrote:

> On Mon, 30 Nov 2015, Marek Polacek wrote:
> 
> > On Sat, Nov 28, 2015 at 08:50:12AM +0100, Richard Biener wrote:
> > > Different approach: after the FE folds (unexpectedly?), scan the result for
> > > SAVE_EXPRs and if found, drop the folding.
> > 
> > Neither this fixes this problem completely, because we simply don't know where
> > those SAVE_EXPRs might be introduced: it might be convert(), but e.g. when I
> > changed the original testcase a tiny bit (added -), then those SAVE_EXPRs were
> > introduced in a different spot (via c_process_stmt_expr -> c_fully_fold).
> 
> So the following "disables" save_expr generation from generic-match.c
> by failing to simplify if save_expr would end up not returning a
> non-save_expr.
> 
> I expect this will make fixing PR68590 difficult (w/o re-introducing
> some fold-const.c code or changing genmatch to "special-case"
> things).
> 
> The other option for this PR is to re-introduce the TREE_SIDE_EFFECTS
> check I removed earlier (to avoid un-CSEing large expressions at
> -O0 for example) and thus only FAIL if the save_expr were needed
> for correctness.

And the following will avoid quite some fallout (eventually).  Testing
as desired change independently.

Richard.

Index: gcc/match.pd
===================================================================
--- gcc/match.pd	(revision 231065)
+++ gcc/match.pd	(working copy)
@@ -1828,15 +1828,14 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT)
  
 /* Simplify comparison of something with itself.  For IEEE
    floating-point, we can only do some of these simplifications.  */
-(simplify
- (eq @0 @0)
- (if (! FLOAT_TYPE_P (TREE_TYPE (@0))
-      || ! HONOR_NANS (TYPE_MODE (TREE_TYPE (@0))))
-  { constant_boolean_node (true, type); }))
-(for cmp (ge le)
+(for cmp (eq ge le)
  (simplify
   (cmp @0 @0)
-  (eq @0 @0)))
+  (if (! FLOAT_TYPE_P (TREE_TYPE (@0))
+       || ! HONOR_NANS (TYPE_MODE (TREE_TYPE (@0))))
+   { constant_boolean_node (true, type); }
+   (if (cmp != EQ_EXPR)
+    (eq @0 @0)))))
 (for cmp (ne gt lt)
  (simplify
   (cmp @0 @0)


> Richard.
> 
> Index: gcc/tree.c
> ===================================================================
> --- gcc/tree.c	(revision 231065)
> +++ gcc/tree.c	(working copy)
> @@ -3231,8 +3231,6 @@ decl_address_ip_invariant_p (const_tree
>     not handle arithmetic; that's handled in skip_simple_arithmetic and
>     tree_invariant_p).  */
>  
> -static bool tree_invariant_p (tree t);
> -
>  static bool
>  tree_invariant_p_1 (tree t)
>  {
> @@ -3282,7 +3280,7 @@ tree_invariant_p_1 (tree t)
>  
>  /* Return true if T is function-invariant.  */
>  
> -static bool
> +bool
>  tree_invariant_p (tree t)
>  {
>    tree inner = skip_simple_arithmetic (t);
> Index: gcc/tree.h
> ===================================================================
> --- gcc/tree.h	(revision 231065)
> +++ gcc/tree.h	(working copy)
> @@ -4320,6 +4320,10 @@ extern tree staticp (tree);
>  
>  extern tree save_expr (tree);
>  
> +/* Return true if T is function-invariant.  */
> +
> +extern bool tree_invariant_p (tree);
> +
>  /* Look inside EXPR into any simple arithmetic operations.  Return the
>     outermost non-arithmetic or non-invariant node.  */
>  
> Index: gcc/genmatch.c
> ===================================================================
> --- gcc/genmatch.c	(revision 231065)
> +++ gcc/genmatch.c	(working copy)
> @@ -3106,7 +3106,9 @@ dt_simplify::gen_1 (FILE *f, int indent,
>  	  else if (is_a <predicate_id *> (opr))
>  	    is_predicate = true;
>  	  /* Search for captures used multiple times in the result expression
> -	     and dependent on TREE_SIDE_EFFECTS emit a SAVE_EXPR.  */
> +	     and check if we can safely evaluate it multiple times.  Otherwise
> +	     fail, avoiding a SAVE_EXPR because that confuses the C FE
> +	     const expression folding.  */
>  	  if (!is_predicate)
>  	    for (int i = 0; i < s->capture_max + 1; ++i)
>  	      {
> @@ -3114,8 +3116,8 @@ dt_simplify::gen_1 (FILE *f, int indent,
>  		  continue;
>  		if (cinfo.info[i].result_use_count > 1)
>  		  fprintf_indent (f, indent,
> -				  "captures[%d] = save_expr (captures[%d]);\n",
> -				  i, i);
> +				  "if (! tree_invariant_p (captures[%d])) "
> +				  "return NULL_TREE;\n", i);
>  	      }
>  	  for (unsigned j = 0; j < e->ops.length (); ++j)
>  	    {
> 

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