Re: [PATCH] Account for devirtualization opportunities in inliner

[Maxim Kuvyrkov <maxim at codesourcery dot com>]

On 20/10/2011, at 10:11 PM, Jan Hubicka wrote:
> static clause_t
> -evaluate_conditions_for_edge (struct cgraph_edge *e, bool inline_p)
> +evaluate_conditions_vals_binfos_for_edge (struct cgraph_edge *e,
> +					  bool inline_p,
> +					  VEC (tree, heap) **known_vals_ptr,
> +					  VEC (tree, heap) **known_binfos_ptr)
> 
> Hmm, I would make clause also returned by reference to be sonsistent and perhaps
> call it something like edge_properties
> since it is not really only about evaulating the clause anymore.

Agree.

> 
> -/* Increase SIZE and TIME for size and time needed to handle all calls in NODE.  */
> +/* Estimate benefit devirtualizing indirect edge IE, provided KNOWN_VALS and
> +   KNOWN_BINFOS.  */
> +
> +static void
> +estimate_edge_devirt_benefit (struct cgraph_edge *ie,
> +			      int *size, int *time, int prob,
> +			      VEC (tree, heap) *known_vals,
> +			      VEC (tree, heap) *known_binfos)
> 
> I think this whole logic should go into estimate_edge_time_and_size.  This way
> we will save all the duplication of scaling logic
> Just add the known_vals/binfos arguments.

Then devirtualization benefit will not be available through estimate_node_size_and_time, which is the primary interface for users of ipa-inline-analysis other than the inliner itself.  I.e., estimate_ipcp_clone_size_and_time, which is the only other user of the analysis at the moment, will not see devirtualization benefit.

> 
> I am not quite sure how to estimate the actual benefits.  estimate_num_insns
> doesn't really make a difference in between direct and indirect calls.
> 
> I see it is good idea to inline more then the destination is known & inlinable.
> This is an example when we have additional knowledge that we want to mix into
> badness metric that does not directly translate to time/size.  There are multiple
> cases like this.  I was thinking of adding kind of bonus metric for this purpose,
> but I would suggest doing this incrementally.

I too thought about this, and decided to keep the bonus metric part to bare minimum in this patch.

> 
> What about
> 1) extending estimate_num_insns wieghts to account direct calls differently
>    from indirect calls (i.e. adding indirect_call cost value into eni wights)
>    I would set it 2 for size metrics and 15 for time metrics for start
> 2) make estimate_edge_time_and_size to subtract difference of those two metrics
>    from edge costs when destination is direct.

OK, I'll try this.

>  @@ -2125,25 +2207,35 @@ estimate_calls_size_and_time (struct cgraph_node *node, int *size, int *time,
> 	    }
> 	  else
> 	    estimate_calls_size_and_time (e->callee, size, time,
> -					  possible_truths);
> +					  possible_truths,
> +					  /* TODO: remap KNOWN_VALS and
> +					     KNOWN_BINFOS to E->CALLEE
> +					     parameters, and use them.  */
> +					  NULL, NULL);
> 
> Remapping should not be needed here - the jump functions are merged after marking edge inline, so jump
> functions in inlined functions actually reffer to the parameters of the function they are inlined to.

I remember it crashing on some testcase and thought the lack of remapping was the cause.  I'll look into this.

Thank you,

--
Maxim Kuvyrkov
CodeSourcery / Mentor Graphics