Re: [trans-mem] cgraph edges vs function cloning

[Jan Hubicka <hubicka@ucw.cz>]

> >              struct cgraph_edge *edge = cgraph_edge (id->src_node, 
> >              orig_stmt);
> POINT_A
> >              int flags;
> >
> >              switch (id->transform_call_graph_edges)
> >                {
> >              case CB_CGE_DUPLICATE:
> >                if (edge)
> >                  cgraph_clone_edge (edge, id->dst_node, stmt,
> >                                           REG_BR_PROB_BASE, 1,
> >                                           edge->frequency, true);
> >                break;
> >
> >              case CB_CGE_MOVE_CLONES:
> >                cgraph_set_call_stmt_including_clones (id->dst_node, 
> >                orig_stmt, stmt);
> >                break;
> >
> >              case CB_CGE_MOVE:
> >                if (edge)
> >                  cgraph_set_call_stmt (edge, stmt);
> POINT_B
> >                break;
> >
> >              default:
> >                gcc_unreachable ();
> >                }
> >
> >            edge = cgraph_edge (id->src_node, orig_stmt);
> POINT_C
> >            /* Constant propagation on argument done during inlining
> >               may create new direct call.  Produce an edge for it.  */
> >            if ((!edge
> >                 || (edge->indirect_call
> >                     && id->transform_call_graph_edges == 
> >                     CB_CGE_MOVE_CLONES))
> >                && is_gimple_call (stmt)
> >                && (fn = gimple_call_fndecl (stmt)) != NULL)
> POINT_D
> 
> This code cannot possibly work.
> 
> We begin by looking up the edge at POINT_A.
> We then move the edge at POINT_B.
> We then look up the edge *again* at POINT_C.
> Ought we be surprised when we do not find the edge at POINT_D?
> 
> After POINT_D, we "fix" the missing edge by creating a new one, which of 
> course is a duplicate, which then of course leads to verification failure.
> 
> I think POINT_B is additionally buggy in that we've just corrupted
> the cgraph node for the source function when we wanted to change the 
> destination function.  I believe we should have done
> 
>   case CB_CGE_MOVE:
>     edge = cgraph_edge (id->dst_node, orig_stmt);
>     cgraph_set_call_stmt (edge, stmt);
>     // Possibly fix up indirect->direct call here.

Yes, this looks fine.  It looks like bug I formery introduced when
moving code for new clones.
> 
> Although, frankly I think it would be easiest to *only* create edges 
> here.  There ought to be no problem doing the cgraph_clone_edge here
> instead of in cgraph_copy_node_for_versioning.  That would still 
> preserve all of the information you wanted that's attached to the edges.
> 
> Thoughts?

There is code in cgraph_copy_node_for_versioning that redirect callers
in the list to new location.  Since clonning might render some code
unreachable and remove edges, this can lead to ICE.  But since this
was formely invented for IPA-CP, that is now using virtual clones,
perhaps we can simply drop this functionality unless you need it in your
branch?

Honza
> 
> 
> r~