From patchwork Tue Aug 3 13:28:09 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Hubicka X-Patchwork-Id: 60735 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 4C843B6F0E for ; Tue, 3 Aug 2010 23:28:22 +1000 (EST) Received: (qmail 16928 invoked by alias); 3 Aug 2010 13:28:20 -0000 Received: (qmail 16915 invoked by uid 22791); 3 Aug 2010 13:28:18 -0000 X-SWARE-Spam-Status: No, hits=-1.7 required=5.0 tests=AWL, BAYES_00, TW_TM, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from nikam-dmz.ms.mff.cuni.cz (HELO nikam.ms.mff.cuni.cz) (195.113.20.16) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 03 Aug 2010 13:28:11 +0000 Received: from localhost (occam.ms.mff.cuni.cz [195.113.18.121]) by nikam.ms.mff.cuni.cz (Postfix) with ESMTP id 1A7919ACF30 for ; Tue, 3 Aug 2010 15:28:09 +0200 (CEST) Received: by localhost (Postfix, from userid 16202) id 12B3C56419E; Tue, 3 Aug 2010 15:28:09 +0200 (CEST) Date: Tue, 3 Aug 2010 15:28:09 +0200 From: Jan Hubicka To: gcc-patches@gcc.gnu.org Subject: PR tree-optimize/45085 (-fpartial-inlining miscompile) Message-ID: <20100803132808.GA18021@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.18 (2008-05-17) Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org hi, in the testcase we decide to split part of fn9 and by mistake we overwrite earlier computed return value. The actual bug is that visit_bb still expect single statement return bb while later I allowed return bb to contain move and return. However as observed by jakub, it does not make sense to always require the split part to compute return value. In some cases return value is already known from header but still we can split out some code. In fact I already handle case of constant returns, this patch makes the support generic and allow the return value to be computed either by header or split part. When it is computed by header, the split part gets empty return statement. Bootstrapped/regtested x86_64-linux, also tested on Mozilla build. OK? Honza * gcc.c-torture/compile/pr45085.c: New testcase. * ipa-split.c (struct split_point): Add split_part_set_retval. (find_retval): Forward declare. (test_nonssa_use, mark_nonssa_use): Special case return by reference. (consider_split): Compute current->split_part_set_retval. (visit_bb): Do not look into return value. (split_function): Handle !split_part_set_retval Index: testsuite/gcc.c-torture/compile/pr45085.c =================================================================== --- testsuite/gcc.c-torture/compile/pr45085.c (revision 0) +++ testsuite/gcc.c-torture/compile/pr45085.c (revision 0) @@ -0,0 +1,45 @@ +/* { dg-options "-O2 -Wuninitialized" } */ +struct S { char *s1; long s2; }; +struct T { int t1; long t2; long t3; }; +extern int fn2 (void); +extern int fn3 (struct T); +extern struct T fn4 (); +extern int fn5 (char **, long *, int); +extern void fn6 (void); +extern void fn7 (void *); +struct S *fn10 (); +static int p; +static void *q; +extern struct T r; + +static struct T +fn8 (struct T x, int y) +{ + struct S *u = fn10 (); + int v = fn5 (&u->s1, &u->s2, 0); + while (1) + { + if (p) +fn6 (); + if (fn3 (x)) +return fn4 (); + if (y & 1) +return r; + v = fn5 (&u->s1, &u->s2, 1); + } +} + +struct T +fn9 (struct T x, int y) +{ + struct T t = fn8 (x, y); + if (fn2 ()) + fn7 (q); + return t; +} + +void * +fn1 (void) +{ + return fn9; +} Index: ipa-split.c =================================================================== --- ipa-split.c (revision 162820) +++ ipa-split.c (working copy) @@ -120,12 +120,18 @@ struct split_point /* Basic blocks we are splitting away. */ bitmap split_bbs; + + /* True when return value is computed on split part and thus it needs + to be returned. */ + bool split_part_set_retval; }; /* Best split point found. */ struct split_point best_split_point; +static tree find_retval (basic_block return_bb); + /* Callback for walk_stmt_load_store_addr_ops. If T is non-ssa automatic variable, check it if it is present in bitmap passed via DATA. */ @@ -141,6 +147,14 @@ test_nonssa_use (gimple stmt ATTRIBUTE_U || (TREE_CODE (t) == RESULT_DECL) || (TREE_CODE (t) == PARM_DECL))) return bitmap_bit_p ((bitmap)data, DECL_UID (t)); + + /* For DECL_BY_REFERENCE, the return value is actually pointer. We want to pretend + that the value pointed to is actual result decl. */ + if (t && (TREE_CODE (t) == MEM_REF || INDIRECT_REF_P (t)) + && TREE_CODE (TREE_OPERAND (t, 0)) == SSA_NAME + && TREE_CODE (SSA_NAME_VAR (TREE_OPERAND (t, 0))) == RESULT_DECL + && DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))) + return bitmap_bit_p ((bitmap)data, DECL_UID (DECL_RESULT (current_function_decl))); return false; } @@ -260,6 +274,7 @@ consider_split (struct split_point *curr gimple_stmt_iterator bsi; unsigned int i; int incomming_freq = 0; + tree retval; if (dump_file && (dump_flags & TDF_DETAILS)) dump_split_point (dump_file, current); @@ -388,6 +403,43 @@ consider_split (struct split_point *curr if (dump_file && (dump_flags & TDF_DETAILS)) fprintf (dump_file, " Accepted!\n"); + /* See if retval used by return bb is computed by header or split part. + When it is computed by split part, we need to produce return statement + in the split part and add code to header to pass it around. + + This is bit tricky to test: + 1) When there is no return_bb or no return value, we always pass + value around. + 2) Invariants are always computed by caller. + 3) For SSA we need to look if defining statement is in header or split part + 4) For non-SSA we need to look where the var is computed. */ + retval = find_retval (return_bb); + if (!retval) + current->split_part_set_retval = true; + else if (is_gimple_min_invariant (retval)) + current->split_part_set_retval = false; + /* Special case is value returned by reference we record as if it was non-ssa + set to result_decl. */ + else if (TREE_CODE (retval) == SSA_NAME + && TREE_CODE (SSA_NAME_VAR (retval)) == RESULT_DECL + && DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))) + current->split_part_set_retval + = bitmap_bit_p (non_ssa_vars, DECL_UID (SSA_NAME_VAR (retval))); + else if (TREE_CODE (retval) == SSA_NAME) + current->split_part_set_retval + = (!SSA_NAME_IS_DEFAULT_DEF (retval) + && (bitmap_bit_p (current->split_bbs, + gimple_bb (SSA_NAME_DEF_STMT (retval))->index) + || gimple_bb (SSA_NAME_DEF_STMT (retval)) == return_bb)); + else if (TREE_CODE (retval) == PARM_DECL) + current->split_part_set_retval = false; + else if (TREE_CODE (retval) == VAR_DECL + || TREE_CODE (retval) == RESULT_DECL) + current->split_part_set_retval + = bitmap_bit_p (non_ssa_vars, DECL_UID (retval)); + else + current->split_part_set_retval = true; + /* At the moment chose split point with lowest frequency and that leaves out smallest size of header. In future we might re-consider this heuristics. */ @@ -516,6 +568,14 @@ mark_nonssa_use (gimple stmt ATTRIBUTE_U if ((TREE_CODE (t) == VAR_DECL && auto_var_in_fn_p (t, current_function_decl)) || (TREE_CODE (t) == RESULT_DECL)) bitmap_set_bit ((bitmap)data, DECL_UID (t)); + + /* For DECL_BY_REFERENCE, the return value is actually pointer. We want to pretend + that the value pointed to is actual result decl. */ + if (t && (TREE_CODE (t) == MEM_REF || INDIRECT_REF_P (t)) + && TREE_CODE (TREE_OPERAND (t, 0)) == SSA_NAME + && TREE_CODE (SSA_NAME_VAR (TREE_OPERAND (t, 0))) == RESULT_DECL + && DECL_BY_REFERENCE (DECL_RESULT (current_function_decl))) + return bitmap_bit_p ((bitmap)data, DECL_UID (DECL_RESULT (current_function_decl))); return false; } @@ -630,7 +690,6 @@ visit_bb (basic_block bb, basic_block re FOR_EACH_EDGE (e, ei, bb->succs) if (e->dest == return_bb) { - bool found_phi = false; for (bsi = gsi_start_phis (return_bb); !gsi_end_p (bsi); gsi_next (&bsi)) { gimple stmt = gsi_stmt (bsi); @@ -640,25 +699,11 @@ visit_bb (basic_block bb, basic_block re continue; if (!is_gimple_reg (gimple_phi_result (stmt))) continue; - found_phi = true; if (TREE_CODE (op) == SSA_NAME) bitmap_set_bit (used_ssa_names, SSA_NAME_VERSION (op)); else can_split &= !mark_nonssa_use (stmt, op, non_ssa_vars); } - if (!gsi_end_p (gsi_last_bb (return_bb))) - { - ssa_op_iter iter; - gimple stmt = gsi_stmt (gsi_last_bb (return_bb)); - tree op; - if (!found_phi) - FOR_EACH_SSA_TREE_OPERAND (op, stmt, iter, SSA_OP_USE) - bitmap_set_bit (used_ssa_names, SSA_NAME_VERSION (op)); - can_split &= !walk_stmt_load_store_addr_ops (stmt, non_ssa_vars, - mark_nonssa_use, - mark_nonssa_use, - mark_nonssa_use); - } } return can_split; } @@ -905,8 +950,55 @@ split_function (struct split_point *spli if (e) split_part_return_p = true; - /* If we return, we will need the return block. */ - if (return_bb != EXIT_BLOCK_PTR && split_part_return_p) + /* Add return block to what will become the split function. + We do not return; no return block is needed. */ + if (!split_part_return_p) + ; + /* We have no return block, so nothing is needed. */ + else if (return_bb == EXIT_BLOCK_PTR) + ; + /* When we do not want to return value, we need to construct + new return block with empty return statement. + FIXME: Once we are able to change return type, we should change function + to return void instead of just outputting function with undefined return + value. For structures this affects quality of codegen. */ + else if (!split_point->split_part_set_retval + && find_retval (return_bb)) + { + bool redirected = true; + basic_block new_return_bb = create_basic_block (NULL, 0, return_bb); + gimple_stmt_iterator gsi = gsi_start_bb (new_return_bb); + gsi_insert_after (&gsi, gimple_build_return (NULL), GSI_NEW_STMT); + while (redirected) + { + redirected = false; + FOR_EACH_EDGE (e, ei, return_bb->preds) + if (bitmap_bit_p (split_point->split_bbs, e->src->index)) + { + new_return_bb->count += e->count; + new_return_bb->frequency += EDGE_FREQUENCY (e); + redirect_edge_and_branch (e, new_return_bb); + redirected = true; + break; + } + } + e = make_edge (new_return_bb, EXIT_BLOCK_PTR, 0); + e->probability = REG_BR_PROB_BASE; + e->count = new_return_bb->count; + bitmap_set_bit (split_point->split_bbs, new_return_bb->index); + /* We change CFG in a way tree-inline is not able to compensate on while + updating PHIs. There are only virtuals in return_bb, so recompute + them. */ + for (gsi = gsi_start_phis (return_bb); !gsi_end_p (gsi);) + { + gimple stmt = gsi_stmt (gsi); + gcc_assert (!is_gimple_reg (gimple_phi_result (stmt))); + mark_sym_for_renaming (SSA_NAME_VAR (PHI_RESULT (stmt))); + gsi_remove (&gsi, false); + } + } + /* When we pass aorund the value, use existing return block. */ + else bitmap_set_bit (split_point->split_bbs, return_bb->index); /* Now create the actual clone. */ @@ -974,15 +1066,7 @@ split_function (struct split_point *spli { real_retval = retval = find_retval (return_bb); - /* See if return value is computed by split part; - function might just return its argument, invariant or undefined - value. In this case we don't need to do any updating. */ - if (real_retval - && !is_gimple_min_invariant (retval) - && (TREE_CODE (retval) != SSA_NAME - || (!SSA_NAME_IS_DEFAULT_DEF (retval) - || DECL_BY_REFERENCE - (DECL_RESULT (current_function_decl))))) + if (real_retval && split_point->split_part_set_retval) { gimple_stmt_iterator psi; @@ -1038,7 +1122,8 @@ split_function (struct split_point *spli else { gimple ret; - if (!VOID_TYPE_P (TREE_TYPE (TREE_TYPE (current_function_decl)))) + if (split_point->split_part_set_retval + && !VOID_TYPE_P (TREE_TYPE (TREE_TYPE (current_function_decl)))) { retval = DECL_RESULT (current_function_decl);