Re: set the correct block info for the call stmt in fnsplit (issue6111050)

[Andrew Pinski <pinskia at gmail dot com>]

On Tue, Apr 24, 2012 at 2:57 PM, Rong Xu <xur@google.com> wrote:
> Hi,
>
> In split_function() (ipa-split.c), for the newly created call stmt,
> its block is set to the outermost scope, i.e.
> DECL_INITIAL(current_function_decl). When we inline this
> partially outlined function, we create the new block based on the
> block for the call stmt (in expand_call_inline()).
> So the new block for the inlined body is in
> parallel to the function top level scope. This bad block structure will
> late result in a bad stack layout.

We do not depend on the block structure any more when dealing with
stack layout for variables in GCC 4.7.0 and above.  I am not saying
your patch is incorrect or not needed.  Just it will not have an
effect on variable stack layout.

Did you have a testcase for the bad stack layout issue?  Or was it too
hard to produce one because the size matters?


>
> This patch fixes the issue by setting the correct block number. It
> starts with the split_point entry bb to find the block stmt in the
> outlined region. The entry_bb maybe an empty BB so we need to follow
> the CFG until we find a non-empty bb.
>
> My early patch tried to use the block info from the first non-empty
> bb in the outlined regision. But that failed bootstrap as some of the
> stmts (assignment stmt) do not have the block info (bug?). So in this
> patch I traverse all the stmts in the bb until getting the block info.
>
> Tested with gcc bootstap.

On which target?

Thanks,
Andrew Pinski

>
> Thanks,
>
> 2012-04-24 Â Rong Xu Â<xur@google.com>
>
> Â Â Â Â* ipa-split.c (split_function): set the correct block for the
> Â Â Â Âcall statement.
>
> Index: ipa-split.c
> ===================================================================
> --- ipa-split.c (revision 186634)
> +++ ipa-split.c (working copy)
> @@ -948,7 +948,7 @@
> Â int num = 0;
> Â struct cgraph_node *node, *cur_node = cgraph_node (current_function_decl);
> Â basic_block return_bb = find_return_bb ();
> - Âbasic_block call_bb;
> + Âbasic_block call_bb, bb;
> Â gimple_stmt_iterator gsi;
> Â gimple call;
> Â edge e;
> @@ -958,6 +958,7 @@
> Â gimple last_stmt = NULL;
> Â unsigned int i;
> Â tree arg;
> + Âtree split_loc_block = NULL;
>
> Â if (dump_file)
> Â Â {
> @@ -1072,6 +1073,33 @@
> Â Â Â Â}
> Â Â }
>
> + Â/* Find the block location of the split region. Â*/
> + Âbb = split_point->entry_bb;
> + Âdo
> + Â Â{
> + Â Â Âbool found = false;
> +
> + Â Â Âfor (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
> + Â Â Â Â{
> + Â Â Â Â Âif (is_gimple_debug(gsi_stmt(gsi)))
> + Â Â Â Â Â Âcontinue;
> + Â Â Â Â Âif ((split_loc_block = gimple_block (gsi_stmt (gsi))))
> + Â Â Â Â Â Â{
> + Â Â Â Â Â Â Âfound = true;
> + Â Â Â Â Â Â Âbreak;
> + Â Â Â Â Â Â}
> + Â Â Â Â}
> + Â Â Âif (found)
> + Â Â Â Âbreak;
> +
> + Â Â Â/* If we reach here, this bb should be an empty unconditional
> + Â Â Â Â or fall-throuth branch. We continue with the succ bb. Â*/
> + Â Â Âbb = single_succ (bb);
> + Â Â}
> + Âwhile (bb && bitmap_bit_p (split_point->split_bbs, bb->index));
> +
> + Âgcc_assert (split_loc_block);
> +
> Â /* Now create the actual clone. Â*/
> Â rebuild_cgraph_edges ();
> Â node = cgraph_function_versioning (cur_node, NULL, NULL, args_to_skip,
> @@ -1115,7 +1143,7 @@
> Â Â Â ÂVEC_replace (tree, args_to_pass, i, arg);
> Â Â Â }
> Â call = gimple_build_call_vec (node->decl, args_to_pass);
> - Âgimple_set_block (call, DECL_INITIAL (current_function_decl));
> + Âgimple_set_block (call, split_loc_block);
>
> Â /* We avoid address being taken on any variable used by split part,
> Â Â Âso return slot optimization is always possible. ÂMoreover this is
>
> --
> This patch is available for review at http://codereview.appspot.com/6111050