From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 9645 invoked by alias); 31 Jul 2017 14:03:33 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 7381 invoked by uid 89); 31 Jul 2017 14:03:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-14.9 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.2 spammy= X-HELO: mx0a-001b2d01.pphosted.com Received: from mx0b-001b2d01.pphosted.com (HELO mx0a-001b2d01.pphosted.com) (148.163.158.5) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 31 Jul 2017 14:03:28 +0000 Received: from pps.filterd (m0098417.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.21/8.16.0.21) with SMTP id v6VDwdst136564 for ; Mon, 31 Jul 2017 10:03:26 -0400 Received: from e18.ny.us.ibm.com (e18.ny.us.ibm.com [129.33.205.208]) by mx0a-001b2d01.pphosted.com with ESMTP id 2c2511bnjm-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Mon, 31 Jul 2017 10:03:26 -0400 Received: from localhost by e18.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 31 Jul 2017 10:03:25 -0400 Received: from b01cxnp22033.gho.pok.ibm.com (9.57.198.23) by e18.ny.us.ibm.com (146.89.104.205) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Mon, 31 Jul 2017 10:03:23 -0400 Received: from b01ledav002.gho.pok.ibm.com (b01ledav002.gho.pok.ibm.com [9.57.199.107]) by b01cxnp22033.gho.pok.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id v6VE3NKF24117288; Mon, 31 Jul 2017 14:03:23 GMT Received: from b01ledav002.gho.pok.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3A40812403F; Mon, 31 Jul 2017 10:00:48 -0400 (EDT) Received: from bigmac.rchland.ibm.com (unknown [9.10.86.172]) by b01ledav002.gho.pok.ibm.com (Postfix) with ESMTPS id 0E8CB124035; Mon, 31 Jul 2017 10:00:48 -0400 (EDT) Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge) From: Bill Schmidt In-Reply-To: <51867599-491F-425C-86ED-73527DD27A7A@linux.vnet.ibm.com> Date: Mon, 31 Jul 2017 14:03:00 -0000 Cc: GCC Patches Content-Transfer-Encoding: quoted-printable References: <51867599-491F-425C-86ED-73527DD27A7A@linux.vnet.ibm.com> To: Richard Biener X-TM-AS-GCONF: 00 x-cbid: 17073114-0044-0000-0000-00000375FD22 X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00007459; HX=3.00000241; KW=3.00000007; PH=3.00000004; SC=3.00000214; SDB=6.00895545; UDB=6.00447910; IPR=6.00675732; BA=6.00005502; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016466; XFM=3.00000015; UTC=2017-07-31 14:03:25 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17073114-0045-0000-0000-000007A40D5B Message-Id: X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-07-31_06:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=1 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000 definitions=main-1707310240 X-IsSubscribed: yes X-SW-Source: 2017-07/txt/msg02028.txt.bz2 > On Jul 31, 2017, at 8:19 AM, Bill Schmidt w= rote: >=20 > That would certainly be much simpler! I'll regstrap it and test it on th= e other > occurrence I've found to be certain. Unfortunately, this fails bootstrap:=20=20 /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c: In function 'rtx_def* emi= t_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode, int, va_l= ist)': /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: error: definition = in block 214 does not dominate use in block 14 emit_library_call_value_1 (int retval, rtx orgfun, rtx value, ^~~~~~~~~~~~~~~~~~~~~~~~~ for SSA_NAME: slsr_772 in statement: slsr_749 =3D PHI <_17(26), slsr_772(14), slsr_334(214)> PHI argument slsr_772 for PHI node slsr_749 =3D PHI <_17(26), slsr_772(14), slsr_334(214)> during GIMPLE pass: slsr /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c:4362:1: internal compiler = error: verify_ssa failed 0x11567cf3 verify_ssa(bool, bool) /home/wschmidt/gcc/gcc-mainline-test/gcc/tree-ssa.c:1186 0x10fc3fff execute_function_todo /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1997 0x10fc277f do_per_function /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:1655 0x10fc42a3 execute_todo /home/wschmidt/gcc/gcc-mainline-test/gcc/passes.c:2044 Please submit a full bug report, with preprocessed source if appropriate. Please include the complete backtrace with any bug report. See for instructions. Not terribly surprising given how sensitive this stuff is. I can look into= why this fails, but looks like it can't be quite this simple, sadly. Bill >=20 > -- Bill >=20 >> On Jul 31, 2017, at 4:15 AM, Richard Biener = wrote: >>=20 >> On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt >> wrote: >>> Hi, >>>=20 >>> PR81354 identifies a latent bug that can happen in SLSR since the >>> conditional candidate support was first added. SLSR relies on the >>> address of a GIMPLE PHI remaining constant during the course of the >>> optimization pass, but it needs to split edges. The use of >>> make_single_succ_edge and reinstall_phi_args in gimple_split_edge >>> causes GIMPLE PHI statements to be temporarily expanded to add a >>> predecessor, and then rebuilt to have the original number of >>> predecessors. The expansion usually, if not always, causes the PHI >>> statement to change address. Thus gimple_split_edge needs to be >>> rewritten to perform in-situ replacement of PHI arguments. >>>=20 >>> The required pieces of make_single_succ_edge have been extracted into >>> two places: make_replacement_pred_edge, and some fixup code at the >>> end of gimple_split_edge. The division is necessary because the >>> destination of the original edge must remember its original >>> predecessors for the switch processing in >>> gimple_redirect_edge_and_branch_1 to work properly. >>>=20 >>> The function gimple_redirect_edge_and_branch was factored into two >>> pieces so that most of it can be used by gimple_split_edge without >>> calling ssa_redirect_edge, which also interferes with PHIs. The >>> useful bits of ssa_redirect_edge are factored out into the next three >>> lines of gimple_split_edge. >>>=20 >>> Similarly, redirect_eh_edge had already been similarly factored into >>> redirect_eh_edge_1 and ssa_redirect_edge. I took advantage of that >>> and exposed redirect_eh_edge_1 for use in gimple_redirect_edge_and_bran= ch_1. >>>=20 >>> I've added the test from PR81354 as a torture test, but as we've seen, >>> small changes elsewhere in the optimizer can easily hide the problem. >>>=20 >>> Bootstrapped and tested on powerpc64le-linux-gnu with no regressions. >>> Is this ok for trunk? Eventually this needs to be backported to GCC 5, >>> 6, and 7 if that's acceptable, since PR81354 was observed on >>> gcc-5-branch. I haven't yet prepared the backports. >>=20 >> I don't like make_replacement_pred_edge too much. Wouldn't it work >> to make sure we first shrink and then re-grow like if we simply do the >> redirect_edge_and_branch before the make_single_succ_edge call? >> At least quick testing shows it fixes the testcase on the GCC 6 branch f= or me. >>=20 >> Index: gcc/tree-cfg.c >> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >> --- gcc/tree-cfg.c (revision 250732) >> +++ gcc/tree-cfg.c (working copy) >> @@ -2753,12 +2753,16 @@ gimple_split_edge (edge edge_in) >> new_bb =3D create_empty_bb (after_bb); >> new_bb->frequency =3D EDGE_FREQUENCY (edge_in); >> new_bb->count =3D edge_in->count; >> + >> + /* First redirect the existing edge to avoid reallocating >> + PHI nodes in dest. */ >> + e =3D redirect_edge_and_branch (edge_in, new_bb); >> + gcc_assert (e =3D=3D edge_in); >> + >> new_edge =3D make_edge (new_bb, dest, EDGE_FALLTHRU); >> new_edge->probability =3D REG_BR_PROB_BASE; >> new_edge->count =3D edge_in->count; >>=20 >> - e =3D redirect_edge_and_branch (edge_in, new_bb); >> - gcc_assert (e =3D=3D edge_in); >> reinstall_phi_args (new_edge, e); >>=20 >> return new_bb; >>=20 >> Sorry for misleading you to a complex solution. >>=20 >> Thanks, >> Richard. >>=20 >>> Thanks, >>> Bill >>>=20 >>>=20 >>> [gcc] >>>=20 >>> 2017-07-30 Bill Schmidt >>>=20 >>> PR tree-optimization/81354 >>> * tree-cfg.c (gimple_redirect_edge_and_branch_1): New decl. >>> (reinstall_phi_args): Delete function. >>> (make_replacement_pred_edge): New function. >>> (gimple_split_edge): Rewrite. >>> (gimple_redirect_edge_and_branch_1): New function, factored >>> from... >>> (gimple_redirect_edge_and_branch): ...here. >>> (split_critical_edges): Don't re-split already split edges. >>> * tree-eh.c (redirect_eh_edge_1): Make visible. >>> * tree-eh.h (redirect_eh_edge_1): Likewise. >>>=20 >>> [gcc/testsuite] >>>=20 >>> 2017-07-30 Bill Schmidt >>>=20 >>> PR tree-optimization/81354 >>> * g++.dg/torture/pr81354.C: New file. >>>=20 >>>=20 >>> Index: gcc/testsuite/g++.dg/torture/pr81354.C >>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>> --- gcc/testsuite/g++.dg/torture/pr81354.C (nonexistent) >>> +++ gcc/testsuite/g++.dg/torture/pr81354.C (working copy) >>> @@ -0,0 +1,24 @@ >>> +// PR81354 reported this test as crashing in a limited range of revisi= ons. >>> +// { dg-do compile } >>> + >>> +struct T { double a; double b; }; >>> + >>> +void foo(T Ad[], int As[2]) >>> +{ >>> + int j; >>> + int i; >>> + int Bs[2] =3D {0,0}; >>> + T Bd[16]; >>> + >>> + for (j =3D 0; j < 4; j++) { >>> + for (i =3D 0; i + 1 <=3D j + 1; i++) { >>> + Ad[i + As[0] * j] =3D Bd[i + Bs[0] * j]; >>> + } >>> + >>> + i =3D j + 1; // <- comment out this line and it does not crash >>> + for (; i + 1 < 5; i++) { >>> + Ad[i + As[0] * j].a =3D 0.0; >>> + Ad[i + As[0] * j].b =3D 0.0; >>> + } >>> + } >>> +} >>> Index: gcc/tree-cfg.c >>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>> --- gcc/tree-cfg.c (revision 250721) >>> +++ gcc/tree-cfg.c (working copy) >>> @@ -154,6 +154,7 @@ static void make_gimple_switch_edges (gswitch *, b >>> static bool make_goto_expr_edges (basic_block); >>> static void make_gimple_asm_edges (basic_block); >>> static edge gimple_redirect_edge_and_branch (edge, basic_block); >>> +static edge gimple_redirect_edge_and_branch_1 (edge, basic_block); >>> static edge gimple_try_redirect_by_replacing_jump (edge, basic_block); >>>=20 >>> /* Various helpers. */ >>> @@ -2776,35 +2777,6 @@ last_and_only_stmt (basic_block bb) >>> return NULL; >>> } >>>=20 >>> -/* Reinstall those PHI arguments queued in OLD_EDGE to NEW_EDGE. */ >>> - >>> -static void >>> -reinstall_phi_args (edge new_edge, edge old_edge) >>> -{ >>> - edge_var_map *vm; >>> - int i; >>> - gphi_iterator phis; >>> - >>> - vec *v =3D redirect_edge_var_map_vector (old_edge); >>> - if (!v) >>> - return; >>> - >>> - for (i =3D 0, phis =3D gsi_start_phis (new_edge->dest); >>> - v->iterate (i, &vm) && !gsi_end_p (phis); >>> - i++, gsi_next (&phis)) >>> - { >>> - gphi *phi =3D phis.phi (); >>> - tree result =3D redirect_edge_var_map_result (vm); >>> - tree arg =3D redirect_edge_var_map_def (vm); >>> - >>> - gcc_assert (result =3D=3D gimple_phi_result (phi)); >>> - >>> - add_phi_arg (phi, arg, new_edge, redirect_edge_var_map_location = (vm)); >>> - } >>> - >>> - redirect_edge_var_map_clear (old_edge); >>> -} >>> - >>> /* Returns the basic block after which the new basic block created >>> by splitting edge EDGE_IN should be placed. Tries to keep the new bl= ock >>> near its "logical" location. This is of most help to humans looking >>> @@ -2825,6 +2797,24 @@ split_edge_bb_loc (edge edge_in) >>> return dest_prev; >>> } >>>=20 >>> +/* Create a single-predecessor edge from SRC to DST, replacing the >>> + predecessor edge E_IN of DST, with flags FLAGS. This is done in >>> + situ so that phis in DST will not get re-allocated. */ >>> + >>> +static edge >>> +make_replacement_pred_edge (basic_block src, basic_block dest, >>> + edge e_in, int flags) >>> +{ >>> + edge e =3D ggc_cleared_alloc (); >>> + n_edges_for_fn (cfun)++; >>> + e->src =3D src; >>> + e->dest =3D dest; >>> + e->flags =3D flags; >>> + vec_safe_push (src->succs, e); >>> + e->dest_idx =3D e_in->dest_idx; >>> + return e; >>> +} >>> + >>> /* Split a (typically critical) edge EDGE_IN. Return the new block. >>> Abort on abnormal edges. */ >>>=20 >>> @@ -2832,7 +2822,7 @@ static basic_block >>> gimple_split_edge (edge edge_in) >>> { >>> basic_block new_bb, after_bb, dest; >>> - edge new_edge, e; >>> + edge e, f; >>>=20 >>> /* Abnormal edges cannot be split. */ >>> gcc_assert (!(edge_in->flags & EDGE_ABNORMAL)); >>> @@ -2841,15 +2831,33 @@ gimple_split_edge (edge edge_in) >>>=20 >>> after_bb =3D split_edge_bb_loc (edge_in); >>>=20 >>> + /* Create a new block, and an edge F from that block to the original >>> + destination. */ >>> new_bb =3D create_empty_bb (after_bb); >>> new_bb->frequency =3D EDGE_FREQUENCY (edge_in); >>> new_bb->count =3D edge_in->count; >>> - new_edge =3D make_single_succ_edge (new_bb, dest, EDGE_FALLTHRU); >>> + f =3D make_replacement_pred_edge (new_bb, dest, edge_in, EDGE_FALLTH= RU); >>>=20 >>> - e =3D redirect_edge_and_branch (edge_in, new_bb); >>> + /* Redirect the original edge to its new successor. */ >>> + e =3D gimple_redirect_edge_and_branch_1 (edge_in, new_bb); >>> gcc_assert (e =3D=3D edge_in); >>> - reinstall_phi_args (new_edge, e); >>> + e->dest =3D new_bb; >>> + vec_safe_push (new_bb->preds, e); >>> + e->dest_idx =3D 0; >>>=20 >>> + /* Fix up the predecessor edge for DEST to now be F. We can't do >>> + this prior to gimple_redirect_edge_and_branch_1 without raising >>> + havoc in the switch code. */ >>> + int idx =3D -1; >>> + for (unsigned int i =3D 0; i < EDGE_COUNT (dest->preds); i++) >>> + if (EDGE_PRED (dest, i) =3D=3D edge_in) >>> + { >>> + idx =3D i; >>> + break; >>> + } >>> + gcc_assert (idx !=3D -1); >>> + EDGE_PRED (dest, idx) =3D f; >>> + >>> return new_bb; >>> } >>>=20 >>> @@ -5740,12 +5748,10 @@ gimple_try_redirect_by_replacing_jump (edge e, = bas >>> return NULL; >>> } >>>=20 >>> +/* Primary worker for gimple_redirect_edge_and_branch. */ >>>=20 >>> -/* Redirect E to DEST. Return NULL on failure. Otherwise, return the >>> - edge representing the redirected branch. */ >>> - >>> static edge >>> -gimple_redirect_edge_and_branch (edge e, basic_block dest) >>> +gimple_redirect_edge_and_branch_1 (edge e, basic_block dest) >>> { >>> basic_block bb =3D e->src; >>> gimple_stmt_iterator gsi; >>> @@ -5759,7 +5765,10 @@ static edge >>> return NULL; >>>=20 >>> if (e->flags & EDGE_EH) >>> - return redirect_eh_edge (e, dest); >>> + { >>> + redirect_eh_edge_1 (e, dest, false); >>> + return e; >>> + } >>>=20 >>> if (e->src !=3D ENTRY_BLOCK_PTR_FOR_FN (cfun)) >>> { >>> @@ -5887,9 +5896,19 @@ static edge >>> gcc_assert (e->flags & EDGE_FALLTHRU); >>> break; >>> } >>> + return e; >>> +} >>>=20 >>> - /* Update/insert PHI nodes as necessary. */ >>> +/* Redirect E to DEST. Return NULL on failure. Otherwise, return the >>> + edge representing the redirected branch. */ >>>=20 >>> +static edge >>> +gimple_redirect_edge_and_branch (edge e, basic_block dest) >>> +{ >>> + edge f =3D gimple_redirect_edge_and_branch_1 (e, dest); >>> + if (f !=3D e) >>> + return f; >>> + >>> /* Now update the edges in the CFG. */ >>> e =3D ssa_redirect_edge (e, dest); >>>=20 >>> @@ -8636,13 +8655,18 @@ split_critical_edges (void) >>> basic_block bb; >>> edge e; >>> edge_iterator ei; >>> + int first_free_block; >>>=20 >>> /* split_edge can redirect edges out of SWITCH_EXPRs, which can get >>> expensive. So we want to enable recording of edge to CASE_LABEL_EX= PR >>> mappings around the calls to split_edge. */ >>> start_recording_case_labels (); >>> + first_free_block =3D last_basic_block_for_fn (cfun); >>> FOR_ALL_BB_FN (bb, cfun) >>> { >>> + /* Don't re-split edges we've already split. */ >>> + if (bb->index >=3D first_free_block) >>> + continue; >>> FOR_EACH_EDGE (e, ei, bb->succs) >>> { >>> if (EDGE_CRITICAL_P (e) && !(e->flags & EDGE_ABNORMAL)) >>> Index: gcc/tree-eh.c >>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>> --- gcc/tree-eh.c (revision 250721) >>> +++ gcc/tree-eh.c (working copy) >>> @@ -2279,7 +2279,7 @@ make_eh_edges (gimple *stmt) >>> If false, we're being called from generic cfg manipulation code and we >>> should preserve our place within the region tree. */ >>>=20 >>> -static void >>> +void >>> redirect_eh_edge_1 (edge edge_in, basic_block new_bb, bool change_regio= n) >>> { >>> eh_landing_pad old_lp, new_lp; >>> Index: gcc/tree-eh.h >>> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >>> --- gcc/tree-eh.h (revision 250721) >>> +++ gcc/tree-eh.h (working copy) >>> @@ -32,6 +32,7 @@ extern int lookup_stmt_eh_lp (gimple *); >>> extern bool make_eh_dispatch_edges (geh_dispatch *); >>> extern void make_eh_edges (gimple *); >>> extern edge redirect_eh_edge (edge, basic_block); >>> +extern void redirect_eh_edge_1 (edge, basic_block, bool); >>> extern void redirect_eh_dispatch_edge (geh_dispatch *, edge, basic_bloc= k); >>> extern bool operation_could_trap_helper_p (enum tree_code, bool, bool, = bool, >>> bool, tree, bool *); >=20