This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: Context sensitive type inheritance graph walking


> Hi,
> 
> sorry it took me so long, but it also took me quite a while to chew
> through.  Please consider posting context diff in cases like this.
> Nevertheless, most of the patch is a nice improvement.

Uhm, sorry. Seems I diffed from a different users.
> > There is cgraph_indirect_call_info that walks GIMPLE code and attempts to
> > determine the context of a given call.  It looks for objects located in
> > declarations (static vars/ automatic vars/parameters), objects passed by
> > invisible references and objects passed as THIS pointers.
> > The second two cases are new, the first case is already done by 
> > gimple_extract_devirt_binfo_from_cst
> 
> and I assume we should really put the context there, rather than
> reconstructing it from the edge.  Of course we must stop overloading
> the offset field for that, are there any other obstacles?

No, i think overloading of offset is the only obstackle.  I just tried to keep
the patch self-contained and do not dive into ipa-prop changes - it is long by
itself.
> > -/* See if BINFO's type match OTR_TYPE.  If so, lookup method
> > -   in vtable of TYPE_BINFO and insert method to NODES array.
> > +/* See if BINFO's type match OUTER_TYPE.  If so, lookup 
> > +   BINFO of subtype of TYPE at OFFSET and in that BINFO find
> > +   method in vtable and insert method to NODES array.
> >     Otherwise recurse to base BINFOs.
> >     This match what get_binfo_at_offset does, but with offset
> >     being unknown.
> 
> This function now needs a comprehensive update of the leading comment,
> we have the offset,  so it is known.  I also dislike the name a lot
> because it does not record binfo, but extracts and records the call
> target from it.  Can we call it something like
> record_target_from_binfo or similar?

OK, record_target_from_binfo works for me.

We still do not know full offset (from start of the type being walked) just
partial offset within one of bases of the type.  But I will try to formulate
the comment better - it is indeed result of incremental updates.
> > -  if (types_same_for_odr (type, otr_type)
> > -      && !pointer_set_insert (matched_vtables, BINFO_VTABLE (type_binfo)))
> > +  if (types_same_for_odr (type, outer_type))
> >      {
> > +      tree inner_binfo = get_binfo_at_offset (type_binfo,
> > +					      offset, otr_type);
> 
> OK, get_binfo_at_offset also traverses BINFO_BASEs, I wonder whether
> we need to iterate over them and recurse when types_same_for_odr
> return false, with offset, won't get_binfo_at_offset just handle both
> cases correctly?

No, it is the difference I described above.

get_binfo_at_offset assume that the offset is from start of the BINFO's type it
is given.  This is not true here.  We have derived_type that has outer_type as a base
that has otr_type at offset inside.

We do not know the offset in between derived_type and outer_type.  This is why one
function wraps the other.
> > +/* Given REF call in FNDECL, determine class of the polymorphic
> > +   call (OTR_TYPE), its token (OTR_TOKEN) and CONTEXT.
> > +   Return pointer to object described by the context  */
> > +
> 
> The return value is never used, Is it ever going to be useful?
> Especially since it can be NULL even in useful cases...

Yes, it is supposed to be used by ipa-prop.  We return non-NULL when
the base may be PARM_DECL that can be furhter propagated through.

> 
> 
> > +tree
> > +get_polymorphic_call_info (tree fndecl,
> > +			   tree ref,
> > +			   tree *otr_type,
> > +			   HOST_WIDE_INT *otr_token,
> > +			   ipa_polymorphic_call_context *context)
> > +{
> > +  tree base_pointer;
> > +  *otr_type = obj_type_ref_class (ref);
> > +  *otr_token = tree_low_cst (OBJ_TYPE_REF_TOKEN (ref), 1);
> > +
> > +  /* Set up basic info in case we find nothing interesting in the analysis.  */
> > +  context->outer_type = *otr_type;
> > +  context->offset = 0;
> > +  base_pointer = OBJ_TYPE_REF_OBJECT (ref);
> > +  context->maybe_derived_type = true;
> > +  context->maybe_in_construction = false;
> > +
> > +  /* Walk SSA for outer object.  */
> > +  do 
> > +    {
> > +      if (TREE_CODE (base_pointer) == SSA_NAME
> > +	  && !SSA_NAME_IS_DEFAULT_DEF (base_pointer)
> > +	  && SSA_NAME_DEF_STMT (base_pointer)
> > +	  && gimple_assign_single_p (SSA_NAME_DEF_STMT (base_pointer)))
> > +	{
> > +	  base_pointer = gimple_assign_rhs1 (SSA_NAME_DEF_STMT (base_pointer));
> > +	  STRIP_NOPS (base_pointer);
> 
> If we want to put the context on the edges, we need to adjust the
> offset here.

I do not follow here, strip nops should not alter offsets, right?
> > +		  context->offset += offset2;
> > +		  base_pointer = NULL;
> > +		  /* Make very conservative assumption that all objects
> > +		     may be in construction. 
> > +		     TODO: ipa-prop already contains code to tell better. 
> > +		     merge it later.  */
> > +		  context->maybe_in_construction = true;
> > +		  context->maybe_derived_type = false;
> > +		  return base_pointer;
> 
> Perhaps just return NULL?

This is intended to be hooked into your code, but indeed the base_pointer =
NULL above is leftover from earlier implementation.
> > +   If FINALp is non-NULL, store true if the list is complette.
> 
> I'd prefer if finalp was called completep, this way it sounds like it
> was dependent on the final keyword, which it isn't.

Well, it originally did, but it is no longer the case, I will rename it.

Thanks!
I will address the comments and update patch.

Honza


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]