> 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
