Re: PATCH: Profile driven specialization of indirect/virtual calls

[Tomas Bily <tbily at suse dot cz>]

Hi,

> +   stmt2 = build2 (GIMPLE_MODIFY_STMT, optype, tmp1, fold_convert (optype, build_addr (direct_call->decl, current_function_decl)));
> 
> Please watch the length of lines.  One of my favorite asepcts of GNU
> coding standard ;)
> +   stmt3 = build3 (COND_EXPR, void_type_node,
> + 	    build2 (NE_EXPR, boolean_type_node, tmp1, tmpv),
> + 	    build1 (GOTO_EXPR, void_type_node, label_decl2),
> + 	    build1 (GOTO_EXPR, void_type_node, label_decl1));
> 
> This should be probably indented in the way so the later builds appear
> after the "(" of toplevel build.
> +   /* Fix eh edges */
> +   region = lookup_stmt_eh_region (stmt1);
> +   if (region >= 0 && !TREE_NOTHROW (new_call))
> 
> tree_could_throw (stmt1) is probably canonical way to test whether
> statement needs EH.   
> + /* Find calls inside STMT for that we want to measure histograms for 
> +    indirect/virtual call optimization. */ 
> + static void
> + tree_indirect_call_to_profile (tree stmt, histogram_values *values)
> 
> There should be vertical space in between comment and function (I know
> it is not in a lot
> of my code, I will try to fix it gradually).
ok. corrected.

> +   else
> +     c_node->needed = true;
> 
> You can't just set c_node->needed, you can use cgraph_mark_needed_node (c_node)
> if you really mean to force it to be output into file.  
> But you should not need this at first place, because the function you get called
> at is being output to final file anyway.  Why have you added this?
It's not needed now. Removed.  


> Index: gcc/libgcov.c
> ===================================================================
> *** gcc/libgcov.c	(revision 119879)
> --- gcc/libgcov.c	(working copy)
> *************** __gcov_one_value_profiler (gcov_type *co
> *** 753,758 ****
> --- 753,768 ----
>   }
>   #endif
>   
> + #ifdef L_gcov_indirect_call_profiler
> + /* Tries to determine the most common value among its inputs. */
> + void
> + __gcov_indirect_call_profiler (gcov_type* counter, gcov_type value, void* cur_func, void* callee_func)
> + {
> +   if (cur_func == callee_func)
> +     __gcov_one_value_profiler (counter, value);
> + }
> 
> You want probably to get __gcov_one_value_profiler inlined here t that is
> intentionally not done by the macro machinery.  Probably
> __gcov_one_value_profiler can be moved into static inline function body visible
> for both.
ok.  

> Otherwise the patch seems fine (I would however like to know why you fiddle
> with needed flag first) THere are discussion about tree-prof.exp changes.
> Please ensure before commiting that the tree-prof.exp your new one is based on
> didn't change.
Still same. 

Bootstraped and tested on i686-linux and x86_64-linux.
OK? 

Tomas

Attachment: indirect-call.patch
Description: Text document