Patchwork Adjust call stmt cost for tailcalls

login
register
mail settings
Submitter Richard Guenther
Date June 20, 2012, 9:49 a.m.
Message ID <alpine.LNX.2.00.1206201148060.5191@zhemvz.fhfr.qr>
Download mbox | patch
Permalink /patch/165971/
State New
Headers show

Comments

Richard Guenther - June 20, 2012, 9:49 a.m.
Tailcalls have no argument setup cost and no return value cost.
This patch adjusts estminate_num_insns to reflect that.

Honza, does this look correct?

Bootstrapped and tested on x86_64-unknown-linux-gnu.

Thanks,
Richard.

2012-06-20  Richard Guenther  <rguenther@suse.de>

	* tree-inline.c (estimate_num_insns): Estimate call cost for
	tailcalls properly.
Jan Hubicka - June 22, 2012, 10:54 p.m.
> 
> Tailcalls have no argument setup cost and no return value cost.
> This patch adjusts estminate_num_insns to reflect that.
> 
> Honza, does this look correct?
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> 
> Thanks,
> Richard.
> 
> 2012-06-20  Richard Guenther  <rguenther@suse.de>
> 
> 	* tree-inline.c (estimate_num_insns): Estimate call cost for
> 	tailcalls properly.

Well, as discussed offline, this change should currently be no-op since
we discover tail calls only very late in the game.

I am not sure I agree that the argument costs are zeroed out. I.e. the
arguments are generally passed the same way as they would be for normal call,
just they are homed at different place of the stack frame.
(note that we mark by the tail call flag far more calls than those that
are really expanded to tailcall because target limitations are checked only
in calls.c).

Finally ipa-cp use estimate_move cost to estimate savings for propagating
and I think there is risk in arriving to negative numbers when costs
are not accounted at all calls.

So I am not sure we want to keep the patch in mainline in the current form...

Honza
Richard Guenther - June 26, 2012, 1:08 p.m.
On Sat, 23 Jun 2012, Jan Hubicka wrote:

> > 
> > Tailcalls have no argument setup cost and no return value cost.
> > This patch adjusts estminate_num_insns to reflect that.
> > 
> > Honza, does this look correct?
> > 
> > Bootstrapped and tested on x86_64-unknown-linux-gnu.
> > 
> > Thanks,
> > Richard.
> > 
> > 2012-06-20  Richard Guenther  <rguenther@suse.de>
> > 
> > 	* tree-inline.c (estimate_num_insns): Estimate call cost for
> > 	tailcalls properly.
> 
> Well, as discussed offline, this change should currently be no-op since
> we discover tail calls only very late in the game.
> 
> I am not sure I agree that the argument costs are zeroed out. I.e. the
> arguments are generally passed the same way as they would be for normal call,
> just they are homed at different place of the stack frame.
> (note that we mark by the tail call flag far more calls than those that
> are really expanded to tailcall because target limitations are checked only
> in calls.c).
> 
> Finally ipa-cp use estimate_move cost to estimate savings for propagating
> and I think there is risk in arriving to negative numbers when costs
> are not accounted at all calls.
> 
> So I am not sure we want to keep the patch in mainline in the current form...

I have reverted the patch.

Richard.

Patch

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c	(revision 188817)
+++ gcc/tree-inline.c	(working copy)
@@ -3611,12 +3611,15 @@  estimate_num_insns (gimple stmt, eni_wei
 	  }
 
 	cost = node ? weights->call_cost : weights->indirect_call_cost;
-	if (gimple_call_lhs (stmt))
-	  cost += estimate_move_cost (TREE_TYPE (gimple_call_lhs (stmt)));
-	for (i = 0; i < gimple_call_num_args (stmt); i++)
+	if (!gimple_call_tail_p (stmt))
 	  {
-	    tree arg = gimple_call_arg (stmt, i);
-	    cost += estimate_move_cost (TREE_TYPE (arg));
+	    if (gimple_call_lhs (stmt))
+	      cost += estimate_move_cost (TREE_TYPE (gimple_call_lhs (stmt)));
+	    for (i = 0; i < gimple_call_num_args (stmt); i++)
+	      {
+		tree arg = gimple_call_arg (stmt, i);
+		cost += estimate_move_cost (TREE_TYPE (arg));
+	      }
 	  }
 	break;
       }