public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Martin Jambor <mjambor@suse.cz>
To: Jan Hubicka <hubicka@ucw.cz>
Cc: GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] New IPA-CP with real function cloning
Date: Fri, 08 Jul 2011 17:37:00 -0000	[thread overview]
Message-ID: <20110708165959.GB19992@virgil.arch.suse.de> (raw)
In-Reply-To: <20110707160307.GG26368@atrey.karlin.mff.cuni.cz>

Hi,

On Thu, Jul 07, 2011 at 06:03:07PM +0200, Jan Hubicka wrote:
> Hi,
> patch is long, so let me review it in more passes.

Fair enough.

> > 
> > 
> > 2011-06-22  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	* ipa-prop.h: Include alloc-pool.h.
> > 	(ipa_lattice_type): Removed.
> > 	(ipcp_value_source): New type.
> > 	(ipcp_value): Likewise.
> > 	(ipcp_values_pool): Declare.
> > 	(ipcp_sources_pool): Likewise.
> > 	(ipa_param_descriptor): Removed.
> > 	(ipcp_lattice): Removed fileds type and constant. Added fields decl,
> > 	values, values_count, contains_variable, bottom, used and virt_call.
> > 	(ipa_node_params): New fields lattices, known_vals,
> > 	clone_for_all_contexts and noe dead, removed fields params and
> > 	count_scale.
> > 	(ipa_get_param): Updated.
> > 	(ipa_param_cannot_devirtualize_p): Removed.
> > 	(ipa_param_types_vec_empty): Likewise.
> > 	(ipa_edge_args): New field next_edge_clone.
> > 	(ipa_func_list): Removed.
> > 	(ipa_init_func_list): Removed declaration.
> > 	(ipa_push_func_to_list_1): Likewise.
> > 	(ipa_pop_func_from_list): Likewise.
> > 	(ipa_push_func_to_list): Removed.
> > 	(ipa_lattice_from_jfunc): Remove declaration.
> > 	(ipa_get_jf_pass_through_result): Declare.
> > 	(ipa_get_jf_ancestor_result): Likewise.
> > 	(ipa_value_from_jfunc): Likewise.
> > 	(ipa_get_lattice): Update.
> > 	(ipa_lat_is_single_const): New function.
> > 	* ipa-prop.c (ipa_push_func_to_list_1): Removed.
> > 	(ipa_init_func_list): Likewise.
> > 	(ipa_pop_func_from_list): Likewise.
> > 	(ipa_get_param_decl_index): Fix coding style.
> > 	(ipa_populate_param_decls): Update to use new lattices.
> > 	(ipa_initialize_node_params): Likewise.
> > 	(visit_ref_for_mod_analysis): Likewise.
> > 	(ipa_analyze_params_uses): Likewise.
> > 	(ipa_free_node_params_substructures): Likewise.
> > 	(ipa_edge_duplication_hook): Add the new edge to the list of edge
> > 	clones.
> > 	(ipa_node_duplication_hook): Update to use new lattices.
> > 	(ipa_free_all_structures_after_ipa_cp): Free alloc pools.
> > 	(ipa_free_all_structures_after_iinln): Likewise.
> > 	(ipa_write_node_info): Update to use new lattices.
> > 	(ipa_read_node_info): Likewise.
> > 	(ipa_get_jf_pass_through_result): New function.
> > 	(ipa_get_jf_ancestor_result): Likewise.
> > 	(ipa_value_from_jfunc): Likewise.
> > 	(ipa_cst_from_jfunc): Reimplemented using ipa_value_from_jfunc.
> > 	* ipa-cp.c: Reimplemented.
> > 	* params.def (PARAM_DEVIRT_TYPE_LIST_SIZE): Removed.
> > 	(PARAM_IPA_CP_VALUE_LIST_SIZE): New parameter.
> > 	* Makefile.in (IPA_PROP_H): Added alloc-pool.h to dependencies.
> > 
> > 	* doc/invoke.texi (devirt-type-list-size): Removed description.
> > 	(ipa-cp-value-list-size): Added description.
> > 
> > 	* testsuite/gcc.dg/ipa/ipa-1.c: Updated testcase dump scan.
> > 	* testsuite/gcc.dg/ipa/ipa-2.c: Likewise.
> > 	* testsuite/gcc.dg/ipa/ipa-3.c: Likewise and made functions static.
> > 	* testsuite/gcc.dg/ipa/ipa-4.c: Updated testcase dump scan.
> > 	* testsuite/gcc.dg/ipa/ipa-5.c: Likewise.
> > 	* testsuite/gcc.dg/ipa/ipa-7.c: Xfail test.
> > 	* testsuite/gcc.dg/ipa/ipa-8.c: Updated testcase dump scan.
> > 	* testsuite/gcc.dg/ipa/ipacost-1.c: Likewise.
> > 	* testsuite/gcc.dg/ipa/ipacost-2.c: Likewise.
> > 	* testsuite/gcc.dg/ipa/ipcp-1.c: New test.
> > 	* testsuite/gcc.dg/ipa/ipcp-2.c: Likewise.
> > 	* testsuite/gcc.dg/tree-ssa/ipa-cp-1.c: Updated testcase.
> 
> > /* Interprocedural analyses.
> >    Copyright (C) 2005, 2007, 2008, 2009, 2010
> 2011
> >    Free Software Foundation, Inc.
> > 
> > 
> > /* The following definitions and interfaces are used by
> >    interprocedural analyses or parameters.  */
> > 
> > /* ipa-prop.c stuff (ipa-cp, indirect inlining):  */
> 
> I was bit thinking about it and probably we could make ipa-prop
> and ipa-inline-analysis to be stand alone analysis passes, instead of
> something called either from inliner or ipa-cp analysis stage. But
> that could be done incrementally.

As I said in the first introductory mail, the summary generation part
is not really affected by this patch in any serious way.

> 
> > 
> > /* A jump function for a callsite represents the values passed as actual
> >    arguments of the callsite. There are three main types of values :
> > 
> >    Pass-through - the caller's formal parameter is passed as an actual
> >                   argument, possibly one simple operation performed on it.
> >    Constant     - a constant (is_gimple_ip_invariant)is passed as an actual
> >                   argument.
> >    Unknown      - neither of the above.
> > 
> >    IPA_JF_CONST_MEMBER_PTR stands for C++ member pointers, it is a special
> >    constant in this regard.  Other constants are represented with IPA_JF_CONST.
> 
> While we are at docs, I would bit expand. It seems to me that for someone not familiar
> with the concept is not clear at all why member pointers are special.
> (i.e. explain that they are non-gimple-regs etc.)

OK

> 
> > 
> >    IPA_JF_ANCESTOR is a special pass-through jump function, which means that
> >    the result is an address of a part of the object pointed to by the formal
> >    parameter to which the function refers.  It is mainly intended to represent
> >    getting addresses of of ancestor fields in C++
> >    (e.g. &this_1(D)->D.1766.D.1756).  Note that if the original pointer is
> >    NULL, ancestor jump function must behave like a simple pass-through.
> > 
> >    Other pass-through functions can either simply pass on an unchanged formal
> >    parameter or can apply one simple binary operation to it (such jump
> >    functions are called polynomial).
> > 
> >    IPA_JF_KNOWN_TYPE is a special type of an "unknown" function that applies
> >    only to pointer parameters.  It means that even though we cannot prove that
> >    the passed value is an interprocedural constant, we still know the exact
> >    type of the containing object which may be valuable for devirtualization.
> > 
> >    Jump functions are computed in ipa-prop.c by function
> >    update_call_notes_after_inlining.  Some information can be lost and jump
> >    functions degraded accordingly when inlining, see
> >    update_call_notes_after_inlining in the same file.  */
> 
> I would add pointer to the original Callahan at al ipa-cp paper.
> I think it is cited from ipa-cp, so just copy it from there.

Well, why not.

> 
> > 
> > /* Structure holding data required to describe a pass-through jump function.  */
> > 
> > struct GTY(()) ipa_pass_through_data
> > {
> >   /* If an operation is to be performed on the original parameter, this is the
> >      second (constant) operand.  */
> >   tree operand;
> >   /* Number of the caller's formal parameter being passed.  */
> >   int formal_id;
> 
> I probably should use this in ipa-inline-analsysi where I call it for some reason operand_num :)

So far I have resisted the urge to rename this but it pre-dates my
involvement with gcc.  I'd like it to be called parm_index but since
we might want to use it also for global variables and we might need
something more complex for also handling parts of aggregates, I left
it for later.

> 
> > 
> > struct ipcp_value;
> 
> I wonder if the jump functions used by several passes and ipcp
> internal types both has o go into the same header?

Well, I originally wanted ipa-prop to provide services to the outside
world like ipa-inline-analysis.c and to have ipa-cp self-contained.
But I guess the data separation is more important.  So if we move
ipa_cst_from_jfunc (and its 3 friends) to ipa-cp and move
ipcp_lattice.decl and ipcp_lattice.used to a special structure, I
might move ipcp_value_source, ipcp_value, and ipcp_lattice altogether
to ipa-cp.

At the moment I'm not sure whether I want to do this as a followup
patch or incorporate it in the changes.  I think I'll start with the
latter and revert to the former if it is too invasive to parts which
have not been touched so far by the change.

> 
> >   /* Time benefit and size cost that specializing the function for this value
> >      can bring about in it's callees (transitively).  */
> >   int prop_time_benefit, prop_size_cost;
> >   /* True if this value is currently on the topo-sort stack.  */
> value ;)
> > /* Lattice describing potential values of a formal parameter of a function and
> >    some of their other properties.  */
> > 
> > struct ipcp_lattice
> > {
> >   /* PARAM_DECL of this parameter.  */
> >   tree decl;

> Why we need param de cl here, when almost everything seems to be
> > using those formal_ids?

To look up the index when you have a decl, for replacement maps and
for dumping, and there may be other uses.

> 
> Please add comment how the latice looks like, it is not quite ovious
> i.e. what is top etc.

OK

> > 
> >   /* The list of known values and types in this lattice.  */
> >   struct ipcp_value *values;
> >   /* Number of known values and types in this lattice.  */
> >   int values_count;
> >   /* The lattice contains a variable component  (in addition to values).  */
> >   bool contains_variable;
> >   /* The value of the lattice is bottom (i.e. variable and unusable for any
> >      propagation).  */
> >   bool bottom;
> >   /* The parameter is used.  */
> >   bool used;
> >   /* There is a virtual call based on this parameter.  */
> >   bool virt_call;
> > };
> > 
> > /* ipa_node_params stores information related to formal parameters of functions
> >    and some other information for interprocedural passes that operate on
> >    parameters (such as ipa-cp).  */
> > struct ipa_node_params
> 
> Hmm, I see, this is why we need to mix the ipacp and ipa-prop together.  Probably it would be
> easier if we had ipa_prop_node_params and ipacp_node-params
> (and I call it summary in ipa-inline that I think is sor of common
> way to call this)

Yes, something like that would be part of the plan to split struct
ipcp_lattice into two.  I know that info and IPA_NODE_REF are not the
best names but I'd like to avoid calling everything a summary :-)

> > {
> >   /* Pointer to an array of structures describing individual formal
> >      parameters.  */
> >   struct ipcp_lattice *lattices;
> 
> Hmm, how we get here around the need to mark this GTY(). I.e are we sure that all the known_vals
> must be referneced from elsewhere at ggc time?

(Scalar) constants that are results of arithmetic jump functions may
not be referenced from elsewhere, everything else is referenced from
the jump functions.  If it is a problem it is already present in the
current IPA-CP.  ipa_node_params and lattices are not GTYed there
either.

> I would also slowly switch those things to VECtors..

Perhaps, but individual lattices are and always have been accessed
through ipa_get_lattice which checks bounds and so there's no big
reason to do that.

> 
> >   /* Only for versioned nodes this field would not be NULL,
> >      it points to the node that IPA cp cloned from.  */
> >   struct cgraph_node *ipcp_orig_node;
> Why not use node->clone_of here?

That would not work if the node was a clone created by some other
pass.  I need to differentiate between clones I create because they do
not have lattices but do have the exact values for individual
parameters in known_vals (which is NULL otherwise).  I should probably
use a flag though, the code I ended up only checks it for NULL anyway.

> > 
> > /* ipa_node_params access functions.  Please use these to access fields that
> >    are or will be shared among various passes.  */
> > 
> > /* Set the number of formal parameters. */
> > 
> > static inline void
> > ipa_set_param_count (struct ipa_node_params *info, int count)
> > {
> >   info->param_count = count;
> > }
> > 
> > /* Return the number of formal parameters. */
> > 
> > static inline int
> > ipa_get_param_count (struct ipa_node_params *info)
> > {
> >   return info->param_count;
> > }

> I see this is the need for DECL pointer in lattice.  I think the
> ipcp latice is separate thing from the parameter summary (that is
> also used by ipa-inline-analysis and friends) so I would make this
> separate VECtor with ipa_set_param_count growing the vector to
> proper size and get_param_count using VEC_length

I'll separate the lattice bits.

> > 
> > /* Return the declaration of Ith formal parameter of the function corresponding
> >    to INFO.  Note there is no setter function as this array is built just once
> >    using ipa_initialize_node_params. */
> > 
> > static inline tree
> > ipa_get_param (struct ipa_node_params *info, int i)
> > {
> >   gcc_assert (i >= 0 && i <= info->param_count);
> >   return info->lattices[i].decl;
> > }
> > 
> > /* Return the used flag corresponding to the Ith formal parameter of
> >    the function associated with INFO.  */
> > 
> > static inline bool
> > ipa_is_param_used (struct ipa_node_params *info, int i)
> > {
> >   gcc_assert (i >= 0 && i <= info->param_count);
> >   return info->lattices[i].used;
> > }

> Similarly for the USED flag, should probably go into VECtor along
> with the DECL pointer.

Right.

> > 
> > /* ipa_edge_args stores information related to a callsite and particularly its
> >    arguments.  It can be accessed by the IPA_EDGE_REF macro.  */
> > typedef struct GTY(()) ipa_edge_args
> 
> probably edge_summary would be my preferred name.

Ugh, this is the current name, we may change it later.  In any event
the name should probably tell that the summary is about parameters.

> 
> > {
> >   /* Next pointer in a linked list of clones of the same function.  */
> >   struct cgraph_edge *next_edge_clone;
> 
> What this is needed for?

For get_info_about_necessary_edges and gather_edges_for_value.  If a
value comes from a node and that is itself cloned, it now may come
from two nodes and so when I iterate over potential sources, I have to
include all such edges.  This is their linked list.

This field could be put into a separate structure because it is not
really part of the summary, it is needed only when the clones are
being created.  I'll do that too.

> > 
> >   /* Number of actual arguments in this callsite.  When set to 0,
> >      this callsite's parameters would not be analyzed by the different
> >      stages of IPA CP.  */
> >   int argument_count;
> >   /* Array of the callsite's jump function of each parameter.  */
> >   struct ipa_jump_func GTY ((length ("%h.argument_count"))) *jump_functions;
> 
> Would be prettier as VECtor.

The patch does not touch places which manipulate jump functions and
I'd prefer to keep it that way.

> 
> > } ipa_edge_args_t;
> > 
> > /* This function ensures the array of node param infos is big enough to
> >    accommodate a structure for all nodes and reallocates it if not.  */
> > 
> > static inline void
> > ipa_check_create_node_params (void)
> 
> this name never realy worked for me (i.e. what is it checking?).
> What about ipa_alloc_node_params or ipa_alloc_resize_node_params?
>

The idea behind the name was that it only allocates memory if
necessary.  But I'd be happy with any of the two suggestions.  But
again, preferably in a separate patch.
 
> 
> I realize that most of changes I sugested above are to the existing ipa-prop
> headers.  I would like to see them done, it depends what seems least painful
> for you - i.e. do them as part of your patch, do them beforehand or
> incrementally after the new ipa-cp is comitted to mainline.
> 
> In particular I would like to see the ipa-cp data (used at propagation stage)
> separated from function summaries (i.e. jump functions, param counts and decl/used
> info).

I will try to do this in this patch because I'm changing the
structures anyway.  Changing names of already existing things or
converting already existing arrays to vectors should wait.

> > /* Interprocedural constant propagation
> >    Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011
> >    Free Software Foundation, Inc.
> >    Contributed by Razya Ladelsky <RAZYA@il.ibm.com>
> Since it is complette rewrite, I would add you there ;)

OK :-)

> > /* Interprocedural constant propagation (IPA-CP).
> > 
> >    The goal of this transformation is to
> > 
> >    1) discover functions which are always invoked with some arguments with the
> >       same known constant values and modify the functions so that the
> >       subsequent optimizations can take advantage of the knowledge, and
> > 
> >    2) create specialized versions of functions transformed in this way if
> >       some parameters are known constants only in certain contexts but the
> >       estimated tradeoff between speedup and cost size is deemed good.
> > 
> >    The algorithm also propagates types and attempts to perform type based
> >    devirtualization.  Types are propagated much like constants.
> 
> I would add that the algorithm is not doing only the traditional ipa-cp propagation,
> but also handles partial specialization and propagate known types
> for devirtualization.

I might call point 2 "partial specialization" but otherwise I believe
the comments above already state exactly that... do they not?

> >    First stage - intraprocedural analysis
> >    =======================================
> > 
> >    This phase computes jump_function and modification flags.
> > 
> >    A jump function for a call-site represents the values passed as an actual
> >    arguments of a given call-site. In principle, there are three types of
> >    values:
> > 
> >    Pass through - the caller's formal parameter is passed as an actual
> >                   argument, plus an operation on it can be performed.
> >    Constant - a constant is passed as an actual argument.
> >    Unknown - neither of the above.
> 
> Propbably should document the new fancy types of jump functions.
> Also mention that return functions are not handled, yet.

They are documented in ipa-prop.h and the comment refers the reader
there (just after the part quoted above).  I think it's better to
describe it at one place.

> > 
> >    Second stage - interprocedural analysis
> >    ========================================
> > 
> >    This stage is itself divided into two phases.  In the first, we propagate
> >    known values over the call graph, in the second, we make cloning decisions.
> 
> I would also explicitely mention that this stage is done differently from the
> classical ipa-cp paper.

OK. (But I reference two papers, we differ from both but much less
from the second one).

> > /* Return true iff the CS is an edge within a strongly connected component as
> >    computed by ipa_reduced_postorder.  */
> > 
> > static inline bool
> > edge_within_scc (struct cgraph_edge *cs)
> > {
> >   struct ipa_dfs_info *caller_dfs = (struct ipa_dfs_info *) cs->caller->aux;
> >   struct ipa_dfs_info *callee_dfs;
> >   struct cgraph_node *callee = cgraph_function_node (cs->callee, NULL);
> 
> since thunks are not transparent for you (and eventually we will want to have
> jump functions for those; passthrough in all but first argument and jump
> function representing the thunk adjustment), you want cgraph_function_or_thunk
> node.

True.

> > 
> > /* Print V which is extracted from a value in a lattice to F.  */
> > 
> > static void
> > print_ipcp_constant_value (FILE * f, tree v)
> > {
> >   if (TREE_CODE (v) == TREE_BINFO)
> >     {
> >       fprintf (f, "BINFO ");
> >       print_generic_expr (f, BINFO_TYPE (v), 0);
> 
> I wish we had some resonable way to visualise the BINFOs...
> It was always completely unclear what really is in them. But this is independent problem.
> > 
> > /* Determine whether it is at all technically possible to create clones of NODE
> >    and store this information in the ipa_node_params structure associated
> >    with NODE.  */
> > 
> > static void
> > determine_versionability (struct cgraph_node *node)
> > {
> >   struct cgraph_edge *edge;
> >   bool res = true;
> > 
> >   /* There are a number of generic reasons functions cannot be versioned.  We
> >      also cannot remove parameters if there are type attributes such as fnspec
> >      present.  */
> >   if (node->alias || node->thunk.thunk_p
> 
> We can version alias if its destination is versionable (i.e. get function_or_thunk node)
> also thunks should be versionable but they are not at the moment, inline summary will
> however have versionable flag cleared.

True.  I considered that but decided to allow their processing later
on, so that we can bisect potential problems to allowing that.

> 
> >       || !inline_summary (node)->versionable
> >       || TYPE_ATTRIBUTES (TREE_TYPE (node->decl))
> 
> This is getting an issue for Fortran that attach the function arguments quite commonly,
> perhaps we should start moving ahead on this and ruling out only the arguments that
> gets can't be preserved.

Yes, for example in 433.milc basically all the functions are
considered not versionable because of this.  I also thought of not
removing parameters for such functions.

> Also you need to test attributes of DECL itself.

Really?  We are not doing it now, nether for IPA-CP nor for IPA-SRA
(which is good at triggering problems with these).

> 
> I think this all should be part of can_change_signature_p code, since we can version
> with all attributes I can thunk of when we don't change signature.
> 
> >       || cgraph_function_body_availability (node) <= AVAIL_OVERWRITABLE)
> >     res =  false;
> >   else
> >     /* Removing arguments doesn't work if the function takes varargs
> >        or use __builtin_apply_args.
> >        FIXME: handle this together with can_change_signature flag.  */
> >     for (edge = node->callees; edge; edge = edge->next_callee)
> >       {
> > 	tree t = edge->callee->decl;
> > 	if (DECL_BUILT_IN_CLASS (t) == BUILT_IN_NORMAL
> > 	    && (DECL_FUNCTION_CODE (t) == BUILT_IN_APPLY_ARGS
> > 		|| DECL_FUNCTION_CODE (t) == BUILT_IN_VA_START))
> 
> can_change_sigunature will also handle apply_args.
> Add VA_START code there, too.  For the other use of this flag (in i386) VA_START
> rules out the change, too, but so far we explicitely tested that in the backend.
> 
> Iguess with these changes, inline_summary (node)->versionable should coincide
> with IPA_NODE_REF (node)->node_versionable making this whole code unnecesary
> (it was supposed to work this way, I am not sure why we now have to versionable
> flags).

That would be nice.  I was wondering why the difference between the
two.  Again, I am yet to decide whether this should be done as a
followup or within this change.

> > 
> > /* Worker callback of cgraph_for_node_and_aliases accumulating statistics of
> >    non-thunk incoming edges to NODE.  */
> > 
> > static bool
> > gather_caller_stats (struct cgraph_node *node, void *data)
> > {
> >   struct caller_statistics *stats = (struct caller_statistics *) data;
> >   struct cgraph_edge *cs;
> > 
> >   for (cs = node->callers; cs; cs = cs->next_caller)
> >     if (cs->caller->thunk.thunk_p)
> >       gather_caller_stats (cs->caller, stats);
> >     else
> >       {
> > 	stats->count_sum += cs->count;
> > 	stats->freq_sum += cs->frequency;
> > 	stats->n_calls++;
> > 	if (cgraph_maybe_hot_edge_p (cs))
> > 	  stats->n_hot_calls ++;
> >       }
> >   return false;


> Why you compute count_sum instead of using node->count?  Is it because you are
> interested in local calls only?

I'm only interested in known calls, that I can redirect, i.e. in "local"
direct ones.

> >   if (!optimize_function_for_speed_p (DECL_STRUCT_FUNCTION (node->decl)))
> 
> When optimizing for size, we could still consider clonning the function if amount
> of code saved at call sites is bigger than the function body..
> Not really important however.

And we do it if the parameters are constants in all contexts.

> >     {
> >       if (dump_file)
> >         fprintf (dump_file, "Not considering %s for cloning; "
> > 		 "optimizing it for size.\n",
> >  	         cgraph_node_name (node));
> >       return false;
> >     }
> > 
> >   init_caller_stats (&stats);
> >   cgraph_for_node_and_aliases (node, gather_caller_stats, &stats, false);
> If you use for_node_thunk_and_aliases you can remove the recursion you do above
> and it will work for aliases of thunk correctly, too ;)

But I wouldn't be able to check the edge for hotness.

> > /* Allocate the arrays in TOPO and topologically sort the nodes into order.  */
> > 
> > static void
> > build_topo_info (struct topo_info *topo)
> toporder is probably more clear about what it means ;)

OK

> > /* Initialize ipcp_lattices.  */
> > 
> > static void
> > initialize_node_lattices (struct cgraph_node *node)
> > {
> >   struct ipa_node_params *info = IPA_NODE_REF (node);
> >   struct cgraph_edge *ie;
> >   bool disable = false, variable = false;
> >   int i;
> > 
> >   /* FIXME: Can we clobber only the first argument of thunks?  */
> >   if (node->alias || node->thunk.thunk_p
> 
> I think sane thing to do here is to not have latices for aliases, since you skip
> them during propagatoin, but have latices for thinks, since they should have their
> own jump functions.

After they are separate, we may try to not have them at all.

> 
> You can probably add sanity check into lattice accesor function that
> is is never given an alias and skip the code disabling ipa-cp bellow
> leaving the data uninitialized.
> 
> >       || ipa_is_called_with_var_arguments (info))
> 
> I always wondered if we need this, given that we check for use of
> stdarg.  Sure writing vararg function without actually using the
> arguments is sort of stupid, but it seems that the code should just
> work.

I have seen ICEs because of this when the user passed an incorrect
number of parameters even though the functions was not vararg.  And I
think that wasn't even with LTO.  But a flag saying "just ignore this
for whatever unspecified reason" might be better, yes.

> 
> (at least with one bounds check in propagation for case function is called with
> too few argumetns)

I don't understand the above.

> 
> Also naturally propagating into stdarg functions is possible.
> 

Sure.  But not really important to worry about it now, is it?

Thanks a lot.  I'll try to coma up with the structure reorganization
soon.

Martin

  reply	other threads:[~2011-07-08 17:00 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-06-23 19:53 Martin Jambor
2011-07-07 16:07 ` Jan Hubicka
2011-07-08 17:37   ` Martin Jambor [this message]
2011-07-08 19:07     ` Jan Hubicka
2011-07-14 14:15       ` Martin Jambor
2011-07-14 21:43         ` Jan Hubicka
2011-07-15 13:37           ` Martin Jambor
2011-07-10 19:44 ` Jan Hubicka
2011-07-14 15:41   ` Martin Jambor
2011-07-14 16:19     ` Jan Hubicka
  -- strict thread matches above, loose matches on Subject: below --
2011-06-15 15:41 Martin Jambor

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110708165959.GB19992@virgil.arch.suse.de \
    --to=mjambor@suse.cz \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=hubicka@ucw.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).