diff mbox

[C/C++] Handle COMPOUND_EXPR in convert_to_* (PR c++/56493)

Message ID 20130617161642.GA2336@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek June 17, 2013, 4:16 p.m. UTC 
Hi!

The following patch shows a performance regression caused by the C++ changes
to evaluate side-effects in x += foo (); first and only then do the
addition.  Previously if x was say int and foo () returned unsigned long
long, convert_to_integer would narrow the addition to unsigned int, but
as we now pass to convert_to_* a COMPOUND_EXPR with the side-effects on the
op0 and addition on op1 of the COMPOUND_EXPR, convert_to_integer doesn't
perform this shortening and unfortunately we don't have any gimple
optimization yet to do this later on.  This patch fixes it by handling
COMPOUND_EXPR in convert_to_* where we care about TREE_CODE of the expr.

Ok for trunk?  Ok for 4.8 as well after a while?

2013-06-17  Jakub Jelinek  <jakub@redhat.com>

	PR c++/56493
	* convert.c (convert_to_real, convert_to_expr, convert_to_complex):
	Handle COMPOUND_EXPR.

	* c-c++-common/pr56493.c: New test.


	Jakub

Comments

Joseph Myers June 17, 2013, 4:54 p.m. UTC | #1 
On Mon, 17 Jun 2013, Jakub Jelinek wrote:

> The following patch shows a performance regression caused by the C++ changes
> to evaluate side-effects in x += foo (); first and only then do the
> addition.  Previously if x was say int and foo () returned unsigned long
> long, convert_to_integer would narrow the addition to unsigned int, but
> as we now pass to convert_to_* a COMPOUND_EXPR with the side-effects on the
> op0 and addition on op1 of the COMPOUND_EXPR, convert_to_integer doesn't
> perform this shortening and unfortunately we don't have any gimple
> optimization yet to do this later on.  This patch fixes it by handling
> COMPOUND_EXPR in convert_to_* where we care about TREE_CODE of the expr.
> 
> Ok for trunk?  Ok for 4.8 as well after a while?

I suppose it's OK to fix the regression, though really we should be 
eliminating these early convert_to_* optimizations (optimizing later on 
GIMPLE if possible) rather than adding to them.
Jason Merrill June 17, 2013, 5:44 p.m. UTC | #2 
On 06/17/2013 12:54 PM, Joseph S. Myers wrote:
> I suppose it's OK to fix the regression, though really we should be
> eliminating these early convert_to_* optimizations (optimizing later on
> GIMPLE if possible) rather than adding to them.

My thought as well.  How hard is it to fix this in gimple fold?

Jason
Jakub Jelinek June 17, 2013, 6:09 p.m. UTC | #3 
On Mon, Jun 17, 2013 at 01:44:25PM -0400, Jason Merrill wrote:
> On 06/17/2013 12:54 PM, Joseph S. Myers wrote:
> >I suppose it's OK to fix the regression, though really we should be
> >eliminating these early convert_to_* optimizations (optimizing later on
> >GIMPLE if possible) rather than adding to them.
> 
> My thought as well.  How hard is it to fix this in gimple fold?

PRs for it are open for almost 3 years now, I think Kai has some patch, but
it hasn't been posted yet, so it is unclear if it would make 4.9.
In any case, it isn't probably something that can be backported to release
branches, while perhaps this simple convert.c change is.

Once we have it in GIMPLE, sure, the change in convert.c can be happily
reverted and also those optimizations removed from there too.

	Jakub
Jason Merrill June 17, 2013, 6:49 p.m. UTC | #4 
On 06/17/2013 02:09 PM, Jakub Jelinek wrote:
> On Mon, Jun 17, 2013 at 01:44:25PM -0400, Jason Merrill wrote:
>> On 06/17/2013 12:54 PM, Joseph S. Myers wrote:
>>> I suppose it's OK to fix the regression, though really we should be
>>> eliminating these early convert_to_* optimizations (optimizing later on
>>> GIMPLE if possible) rather than adding to them.
>>
>> My thought as well.  How hard is it to fix this in gimple fold?
>
> PRs for it are open for almost 3 years now, I think Kai has some patch, but
> it hasn't been posted yet, so it is unclear if it would make 4.9.

Kai, what's the status of this patch?

Jason
Richard Biener June 18, 2013, 8:20 a.m. UTC | #5 
On Mon, Jun 17, 2013 at 6:54 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Mon, 17 Jun 2013, Jakub Jelinek wrote:
>
>> The following patch shows a performance regression caused by the C++ changes
>> to evaluate side-effects in x += foo (); first and only then do the
>> addition.  Previously if x was say int and foo () returned unsigned long
>> long, convert_to_integer would narrow the addition to unsigned int, but
>> as we now pass to convert_to_* a COMPOUND_EXPR with the side-effects on the
>> op0 and addition on op1 of the COMPOUND_EXPR, convert_to_integer doesn't
>> perform this shortening and unfortunately we don't have any gimple
>> optimization yet to do this later on.  This patch fixes it by handling
>> COMPOUND_EXPR in convert_to_* where we care about TREE_CODE of the expr.
>>
>> Ok for trunk?  Ok for 4.8 as well after a while?
>
> I suppose it's OK to fix the regression, though really we should be
> eliminating these early convert_to_* optimizations (optimizing later on
> GIMPLE if possible) rather than adding to them.

I agree.  The correct place for this is in tree-ssa-forwprop.c (for now).

Richard.
diff mbox

Patch

--- gcc/convert.c.jj	2013-05-13 09:44:53.000000000 +0200
+++ gcc/convert.c	2013-06-16 12:16:13.754108523 +0200
@@ -95,6 +95,15 @@  convert_to_real (tree type, tree expr)
   enum built_in_function fcode = builtin_mathfn_code (expr);
   tree itype = TREE_TYPE (expr);
 
+  if (TREE_CODE (expr) == COMPOUND_EXPR)
+    {
+      tree t = convert_to_real (type, TREE_OPERAND (expr, 1));
+      if (t == TREE_OPERAND (expr, 1))
+	return expr;
+      return build2_loc (EXPR_LOCATION (expr), COMPOUND_EXPR, TREE_TYPE (t),
+			 TREE_OPERAND (expr, 0), t);
+    }    
+
   /* Disable until we figure out how to decide whether the functions are
      present in runtime.  */
   /* Convert (float)sqrt((double)x) where x is float into sqrtf(x) */
@@ -366,6 +375,15 @@  convert_to_integer (tree type, tree expr
       return error_mark_node;
     }
 
+  if (ex_form == COMPOUND_EXPR)
+    {
+      tree t = convert_to_integer (type, TREE_OPERAND (expr, 1));
+      if (t == TREE_OPERAND (expr, 1))
+	return expr;
+      return build2_loc (EXPR_LOCATION (expr), COMPOUND_EXPR, TREE_TYPE (t),
+			 TREE_OPERAND (expr, 0), t);
+    }    
+
   /* Convert e.g. (long)round(d) -> lround(d).  */
   /* If we're converting to char, we may encounter differing behavior
      between converting from double->char vs double->long->char.
@@ -854,6 +872,14 @@  convert_to_complex (tree type, tree expr
 
 	if (TYPE_MAIN_VARIANT (elt_type) == TYPE_MAIN_VARIANT (subtype))
 	  return expr;
+	else if (TREE_CODE (expr) == COMPOUND_EXPR)
+	  {
+	    tree t = convert_to_complex (type, TREE_OPERAND (expr, 1));
+	    if (t == TREE_OPERAND (expr, 1))
+	      return expr;
+	    return build2_loc (EXPR_LOCATION (expr), COMPOUND_EXPR,
+			       TREE_TYPE (t), TREE_OPERAND (expr, 0), t);
+	  }    
 	else if (TREE_CODE (expr) == COMPLEX_EXPR)
 	  return fold_build2 (COMPLEX_EXPR, type,
 			      convert (subtype, TREE_OPERAND (expr, 0)),
--- gcc/testsuite/c-c++-common/pr56493.c.jj	2013-06-17 10:24:36.891659600 +0200
+++ gcc/testsuite/c-c++-common/pr56493.c	2013-06-17 10:24:33.164720149 +0200
@@ -0,0 +1,16 @@ 
+/* PR c++/56493 */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-tree-gimple" } */
+
+unsigned long long bar (void);
+int x;
+
+void
+foo (void)
+{
+  x += bar ();
+}
+
+/* Verify we narrow the addition from unsigned long long to unsigned int type.  */
+/* { dg-final { scan-tree-dump "  (\[a-zA-Z._0-9]*) = \\(unsigned int\\) \[^;\n\r]*;.*  (\[a-zA-Z._0-9]*) = \\(unsigned int\\) \[^;\n\r]*;.* = \\1 \\+ \\2;" "gimple" { target { ilp32 || lp64 } } } } */
+/* { dg-final { cleanup-tree-dump "gimple" } } */