From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 8817 invoked by alias); 28 Jul 2009 17:16:13 -0000 Received: (qmail 8799 invoked by uid 22791); 28 Jul 2009 17:16:09 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00 X-Spam-Check-By: sourceware.org Received: from atrey.karlin.mff.cuni.cz (HELO atrey.karlin.mff.cuni.cz) (195.113.26.193) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 28 Jul 2009 17:16:03 +0000 Received: by atrey.karlin.mff.cuni.cz (Postfix, from userid 4018) id 2867BF0264; Tue, 28 Jul 2009 19:16:01 +0200 (CEST) Date: Tue, 28 Jul 2009 17:16:00 -0000 From: Jan Hubicka To: Richard Henderson Cc: Jan Hubicka , jh@suse.cz, gcc@gcc.gnu.org Subject: Re: [trans-mem] cgraph edges vs function cloning Message-ID: <20090728171601.GB7178@atrey.karlin.mff.cuni.cz> References: <4A67BF29.9090208@twiddle.net> <20090723172828.GA20017@atrey.karlin.mff.cuni.cz> <4A6E333E.7060402@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4A6E333E.7060402@redhat.com> User-Agent: Mutt/1.5.13 (2006-08-11) X-IsSubscribed: yes Mailing-List: contact gcc-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-owner@gcc.gnu.org X-SW-Source: 2009-07/txt/msg00587.txt.bz2 > > 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~