> Hello Josef,
>
> On Tue, Nov 05 2024, Josef Melcr wrote:
> > Hi!
> >
> > On 11/5/24 12:06, Martin Jambor wrote:
> >> +/* Copy information from SRC_JF to DST_JF which correstpond to call graph
> >> edges
> >> + SRC and DST. */
> >> +
> >> +static void
> >> +ipa_duplicate_jump_function (cgraph_edge *src, cgraph_edge *dst,
> >> + ipa_jump_func *src_jf, ipa_jump_func *dst_jf)
> >
> > I am not sure this function copies over all necessary information, e.g.
> > jump function type. It will work fine when used in the duplication hook,
> > as the dst jump functions are created through
> >
> > new_args->jump_functions = vec_safe_copy (old_args->jump_functions);
> >
> > so all other information is copied over, but I don't think that's the
> > case when using this function on its own. When a jump function is
> > constructed through different means, will the contents of the jump
> > functions still match ?
>
> Thanks for reading through the patch. I am happy that I was wrong when
> reviewing the current situation rather then when writing the original
> code. Though of course mainly I need to pay more attention to details.
>
> We still want to have the split for the purposes of propagating stuff to
> OpenMP outlined regions, and for that I have prepared the patch below.
> I am just not sure whether it makes sense to push this now or wait for
> stage1 of GCC 16. The patch does pass bootstrap and testing on
> x86_64-linux.
>
> Honza, any preferences? (Independently of whether it is OK to be
> pushed.)
I think it is fine to commit it now. It makes it easier to maintain the
branch. Patch is OK.
Honza
>
> Thanks,
>
> Martin
>
> ---------- 8< -------------------- 8< -------------------- 8< ----------
>
> When reviewing various IPA bits and pieces I have falsely assumed
> that jump function duplication misses copying important bits because
> it relies on vec_safe_copy-ing all data in the vector of jump
> functions and then just fixes up the few fields it needs to.
>
> Perhaps more importantly, we do want a function to copy one individual
> jump function to form jump functions for planned call-graph edges that
> model transfer of control to OpenMP outlined regions through calls to
> gomp functions.
>
> Therefore, this patch introduces such function and makes
> ipa_edge_args_sum_t::duplicate just allocate the new vectors and then
> uses the new function to copy the data.
>
> gcc/ChangeLog:
>
> 2024-11-12 Martin Jambor <mjam...@suse.cz>
>
> * ipa-prop.cc (ipa_duplicate_jump_function): New function.
> (ipa_edge_args_sum_t::duplicate): Move individual jump function
> copying to ipa_duplicate_jump_function.
> ---
> gcc/ipa-prop.cc | 186 ++++++++++++++++++++++++++++--------------------
> 1 file changed, 110 insertions(+), 76 deletions(-)
>
> diff --git a/gcc/ipa-prop.cc b/gcc/ipa-prop.cc
> index 599181d0a94..e1c4c798f3f 100644
> --- a/gcc/ipa-prop.cc
> +++ b/gcc/ipa-prop.cc
> @@ -4497,99 +4497,96 @@ ipa_edge_args_sum_t::remove (cgraph_edge *cs,
> ipa_edge_args *args)
> }
> }
>
> -/* Method invoked when an edge is duplicated. Copy ipa_edge_args and adjust
> - reference count data strucutres accordingly. */
> +/* Copy information from SRC_JF to DST_JF which correstpond to call graph
> edges
> + SRC and DST. */
>
> -void
> -ipa_edge_args_sum_t::duplicate (cgraph_edge *src, cgraph_edge *dst,
> - ipa_edge_args *old_args, ipa_edge_args
> *new_args)
> +static void
> +ipa_duplicate_jump_function (cgraph_edge *src, cgraph_edge *dst,
> + ipa_jump_func *src_jf, ipa_jump_func *dst_jf)
> {
> - unsigned int i;
> + dst_jf->agg.items = vec_safe_copy (src_jf->agg.items);
> + dst_jf->agg.by_ref = src_jf->agg.by_ref;
>
> - new_args->jump_functions = vec_safe_copy (old_args->jump_functions);
> - if (old_args->polymorphic_call_contexts)
> - new_args->polymorphic_call_contexts
> - = vec_safe_copy (old_args->polymorphic_call_contexts);
> + /* We can avoid calling ipa_set_jfunc_vr since it would only look up the
> + place in the hash_table where the source m_vr resides. */
> + dst_jf->m_vr = src_jf->m_vr;
>
> - for (i = 0; i < vec_safe_length (old_args->jump_functions); i++)
> + if (src_jf->type == IPA_JF_CONST)
> {
> - struct ipa_jump_func *src_jf = ipa_get_ith_jump_func (old_args, i);
> - struct ipa_jump_func *dst_jf = ipa_get_ith_jump_func (new_args, i);
> + ipa_set_jf_cst_copy (dst_jf, src_jf);
> + struct ipa_cst_ref_desc *src_rdesc = jfunc_rdesc_usable (src_jf);
>
> - dst_jf->agg.items = vec_safe_copy (dst_jf->agg.items);
> -
> - if (src_jf->type == IPA_JF_CONST)
> + if (!src_rdesc)
> + dst_jf->value.constant.rdesc = NULL;
> + else if (src->caller == dst->caller)
> {
> - struct ipa_cst_ref_desc *src_rdesc = jfunc_rdesc_usable (src_jf);
> -
> - if (!src_rdesc)
> - dst_jf->value.constant.rdesc = NULL;
> - else if (src->caller == dst->caller)
> + /* Creation of a speculative edge. If the source edge is the one
> + grabbing a reference, we must create a new (duplicate)
> + reference description. Otherwise they refer to the same
> + description corresponding to a reference taken in a function
> + src->caller is inlined to. In that case we just must
> + increment the refcount. */
> + if (src_rdesc->cs == src)
> {
> - /* Creation of a speculative edge. If the source edge is the one
> - grabbing a reference, we must create a new (duplicate)
> - reference description. Otherwise they refer to the same
> - description corresponding to a reference taken in a function
> - src->caller is inlined to. In that case we just must
> - increment the refcount. */
> - if (src_rdesc->cs == src)
> - {
> - symtab_node *n = symtab_node_for_jfunc (src_jf);
> - gcc_checking_assert (n);
> - ipa_ref *ref
> - = src->caller->find_reference (n, src->call_stmt,
> - src->lto_stmt_uid,
> - IPA_REF_ADDR);
> - gcc_checking_assert (ref);
> - dst->caller->clone_reference (ref, ref->stmt);
> + symtab_node *n = symtab_node_for_jfunc (src_jf);
> + gcc_checking_assert (n);
> + ipa_ref *ref
> + = src->caller->find_reference (n, src->call_stmt,
> + src->lto_stmt_uid,
> + IPA_REF_ADDR);
> + gcc_checking_assert (ref);
> + dst->caller->clone_reference (ref, ref->stmt);
>
> - ipa_cst_ref_desc *dst_rdesc = ipa_refdesc_pool.allocate ();
> - dst_rdesc->cs = dst;
> - dst_rdesc->refcount = src_rdesc->refcount;
> - dst_rdesc->next_duplicate = NULL;
> - dst_jf->value.constant.rdesc = dst_rdesc;
> - }
> - else
> - {
> - src_rdesc->refcount++;
> - dst_jf->value.constant.rdesc = src_rdesc;
> - }
> - }
> - else if (src_rdesc->cs == src)
> - {
> - struct ipa_cst_ref_desc *dst_rdesc = ipa_refdesc_pool.allocate ();
> + ipa_cst_ref_desc *dst_rdesc = ipa_refdesc_pool.allocate ();
> dst_rdesc->cs = dst;
> dst_rdesc->refcount = src_rdesc->refcount;
> - dst_rdesc->next_duplicate = src_rdesc->next_duplicate;
> - src_rdesc->next_duplicate = dst_rdesc;
> + dst_rdesc->next_duplicate = NULL;
> dst_jf->value.constant.rdesc = dst_rdesc;
> }
> else
> {
> - struct ipa_cst_ref_desc *dst_rdesc;
> - /* This can happen during inlining, when a JFUNC can refer to a
> - reference taken in a function up in the tree of inline clones.
> - We need to find the duplicate that refers to our tree of
> - inline clones. */
> -
> - gcc_assert (dst->caller->inlined_to);
> - for (dst_rdesc = src_rdesc->next_duplicate;
> - dst_rdesc;
> - dst_rdesc = dst_rdesc->next_duplicate)
> - {
> - struct cgraph_node *top;
> - top = dst_rdesc->cs->caller->inlined_to
> - ? dst_rdesc->cs->caller->inlined_to
> - : dst_rdesc->cs->caller;
> - if (dst->caller->inlined_to == top)
> - break;
> - }
> - gcc_assert (dst_rdesc);
> - dst_jf->value.constant.rdesc = dst_rdesc;
> + src_rdesc->refcount++;
> + dst_jf->value.constant.rdesc = src_rdesc;
> }
> }
> - else if (dst_jf->type == IPA_JF_PASS_THROUGH
> - && src->caller == dst->caller)
> + else if (src_rdesc->cs == src)
> + {
> + struct ipa_cst_ref_desc *dst_rdesc = ipa_refdesc_pool.allocate ();
> + dst_rdesc->cs = dst;
> + dst_rdesc->refcount = src_rdesc->refcount;
> + dst_rdesc->next_duplicate = src_rdesc->next_duplicate;
> + src_rdesc->next_duplicate = dst_rdesc;
> + dst_jf->value.constant.rdesc = dst_rdesc;
> + }
> + else
> + {
> + struct ipa_cst_ref_desc *dst_rdesc;
> + /* This can happen during inlining, when a JFUNC can refer to a
> + reference taken in a function up in the tree of inline clones.
> + We need to find the duplicate that refers to our tree of
> + inline clones. */
> +
> + gcc_assert (dst->caller->inlined_to);
> + for (dst_rdesc = src_rdesc->next_duplicate;
> + dst_rdesc;
> + dst_rdesc = dst_rdesc->next_duplicate)
> + {
> + struct cgraph_node *top;
> + top = dst_rdesc->cs->caller->inlined_to
> + ? dst_rdesc->cs->caller->inlined_to
> + : dst_rdesc->cs->caller;
> + if (dst->caller->inlined_to == top)
> + break;
> + }
> + gcc_assert (dst_rdesc);
> + dst_jf->value.constant.rdesc = dst_rdesc;
> + }
> + }
> + else if (src_jf->type == IPA_JF_PASS_THROUGH)
> + {
> + dst_jf->type = IPA_JF_PASS_THROUGH;
> + dst_jf->value.pass_through = src_jf->value.pass_through;
> + if (src->caller == dst->caller)
> {
> struct cgraph_node *inline_root = dst->caller->inlined_to
> ? dst->caller->inlined_to : dst->caller;
> @@ -4604,6 +4601,43 @@ ipa_edge_args_sum_t::duplicate (cgraph_edge *src,
> cgraph_edge *dst,
> }
> }
> }
> + else if (src_jf->type == IPA_JF_ANCESTOR)
> + {
> + dst_jf->type = IPA_JF_ANCESTOR;
> + dst_jf->value.ancestor = src_jf->value.ancestor;
> + }
> + else
> + gcc_assert (src_jf->type == IPA_JF_UNKNOWN);
> +}
> +
> +/* Method invoked when an edge is duplicated. Copy ipa_edge_args and adjust
> + reference count data strucutres accordingly. */
> +
> +void
> +ipa_edge_args_sum_t::duplicate (cgraph_edge *src, cgraph_edge *dst,
> + ipa_edge_args *old_args, ipa_edge_args
> *new_args)
> +{
> + unsigned int i;
> +
> + if (old_args->polymorphic_call_contexts)
> + new_args->polymorphic_call_contexts
> + = vec_safe_copy (old_args->polymorphic_call_contexts);
> +
> + if (!vec_safe_length (old_args->jump_functions))
> + {
> + new_args->jump_functions = NULL;
> + return;
> + }
> + vec_safe_grow_cleared (new_args->jump_functions,
> + old_args->jump_functions->length (), true);
> +
> + for (i = 0; i < vec_safe_length (old_args->jump_functions); i++)
> + {
> + struct ipa_jump_func *src_jf = ipa_get_ith_jump_func (old_args, i);
> + struct ipa_jump_func *dst_jf = ipa_get_ith_jump_func (new_args, i);
> +
> + ipa_duplicate_jump_function (src, dst, src_jf, dst_jf);
> + }
> }
>
> /* Analyze newly added function into callgraph. */
> --
> 2.47.0
>
>
