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

[Richard Henderson <rth@redhat.com>]

>               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.

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?


r~