On Aug 2, 2017, at 3:09 AM, Richard Biener <richard.guent...@gmail.com> wrote: > > On Tue, Aug 1, 2017 at 5:56 PM, Bill Schmidt > <wschm...@linux.vnet.ibm.com> wrote: >> On Aug 1, 2017, at 8:50 AM, Bill Schmidt <wschm...@linux.vnet.ibm.com> wrote: >>> >>> On Aug 1, 2017, at 7:44 AM, Bill Schmidt <wschm...@linux.vnet.ibm.com> >>> wrote: >>>> >>>>> >>>>> On Aug 1, 2017, at 3:46 AM, Richard Biener <richard.guent...@gmail.com> >>>>> wrote: >>>>> >>>>> On Mon, Jul 31, 2017 at 4:03 PM, Bill Schmidt >>>>> <wschm...@linux.vnet.ibm.com> wrote: >>>>>> >>>>>>> On Jul 31, 2017, at 8:19 AM, Bill Schmidt <wschm...@linux.vnet.ibm.com> >>>>>>> wrote: >>>>>>> >>>>>>> That would certainly be much simpler! I'll regstrap it and test it on >>>>>>> the other >>>>>>> occurrence I've found to be certain. >>>>>> >>>>>> Unfortunately, this fails bootstrap: >>>>>> >>>>>> /home/wschmidt/gcc/gcc-mainline-test/gcc/calls.c: In function 'rtx_def* >>>>>> emit_library_call_value_1(int, rtx, rtx, libcall_type, machine_mode, >>>>>> int, va_list)': >>>>>> /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 = PHI <_17(26), slsr_772(14), slsr_334(214)> >>>>>> PHI argument >>>>>> slsr_772 >>>>>> for PHI node >>>>>> slsr_749 = 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 <https://gcc.gnu.org/bugs/> 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. >>>>> >>>>> Intersting ... a dg-torture.exp run was clean for me (all I >>>>> tested...). So yes, can you >>>>> check what fails? Maybe run the testsuite with the stage1 compiler. >>>> >>>> Looks like it's the stage1 that doesn't build. I think the difference is >>>> that I was building trunk and you were building 6. I'll try to look into >>>> it later today after I get through some meetings. >>> >>> Sorry, no, it was stage 2 where the failure occurs. >> >> Ah, "good" news. I believe the bootstrap failure with this change is another >> rediscovery of https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81503, which >> explains why it wasn't seen for GCC 6. I'll work on getting that fixed and >> then try this again. > > Note I very much would like to "fix" split_edge on trunk, on release branches > I'd appreciate if a simpler fix inside SLSR works, like using > gsi_insert_on_edge > to avoid splitting edges during the iteration.
Sure, I'll look into it. Bill > > Richard. > >> Bill >> >>> >>> Bill >>> >>>> >>>> Bill >>>>> >>>>> Richard. >>>>> >>>>>> Bill >>>>>> >>>>>>> >>>>>>> -- Bill >>>>>>> >>>>>>>> On Jul 31, 2017, at 4:15 AM, Richard Biener >>>>>>>> <richard.guent...@gmail.com> wrote: >>>>>>>> >>>>>>>> On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt >>>>>>>> <wschm...@linux.vnet.ibm.com> wrote: >>>>>>>>> Hi, >>>>>>>>> >>>>>>>>> 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. >>>>>>>>> >>>>>>>>> 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. >>>>>>>>> >>>>>>>>> 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. >>>>>>>>> >>>>>>>>> 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_branch_1. >>>>>>>>> >>>>>>>>> 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. >>>>>>>>> >>>>>>>>> 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. >>>>>>>> >>>>>>>> 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 >>>>>>>> for me. >>>>>>>> >>>>>>>> Index: gcc/tree-cfg.c >>>>>>>> =================================================================== >>>>>>>> --- gcc/tree-cfg.c (revision 250732) >>>>>>>> +++ gcc/tree-cfg.c (working copy) >>>>>>>> @@ -2753,12 +2753,16 @@ gimple_split_edge (edge edge_in) >>>>>>>> new_bb = create_empty_bb (after_bb); >>>>>>>> new_bb->frequency = EDGE_FREQUENCY (edge_in); >>>>>>>> new_bb->count = edge_in->count; >>>>>>>> + >>>>>>>> + /* First redirect the existing edge to avoid reallocating >>>>>>>> + PHI nodes in dest. */ >>>>>>>> + e = redirect_edge_and_branch (edge_in, new_bb); >>>>>>>> + gcc_assert (e == edge_in); >>>>>>>> + >>>>>>>> new_edge = make_edge (new_bb, dest, EDGE_FALLTHRU); >>>>>>>> new_edge->probability = REG_BR_PROB_BASE; >>>>>>>> new_edge->count = edge_in->count; >>>>>>>> >>>>>>>> - e = redirect_edge_and_branch (edge_in, new_bb); >>>>>>>> - gcc_assert (e == edge_in); >>>>>>>> reinstall_phi_args (new_edge, e); >>>>>>>> >>>>>>>> return new_bb; >>>>>>>> >>>>>>>> Sorry for misleading you to a complex solution. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Richard. >>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Bill >>>>>>>>> >>>>>>>>> >>>>>>>>> [gcc] >>>>>>>>> >>>>>>>>> 2017-07-30 Bill Schmidt <wschm...@linux.vnet.ibm.com> >>>>>>>>> >>>>>>>>> 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. >>>>>>>>> >>>>>>>>> [gcc/testsuite] >>>>>>>>> >>>>>>>>> 2017-07-30 Bill Schmidt <wschm...@linux.vnet.ibm.com> >>>>>>>>> >>>>>>>>> PR tree-optimization/81354 >>>>>>>>> * g++.dg/torture/pr81354.C: New file. >>>>>>>>> >>>>>>>>> >>>>>>>>> Index: gcc/testsuite/g++.dg/torture/pr81354.C >>>>>>>>> =================================================================== >>>>>>>>> --- 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 >>>>>>>>> revisions. >>>>>>>>> +// { dg-do compile } >>>>>>>>> + >>>>>>>>> +struct T { double a; double b; }; >>>>>>>>> + >>>>>>>>> +void foo(T Ad[], int As[2]) >>>>>>>>> +{ >>>>>>>>> + int j; >>>>>>>>> + int i; >>>>>>>>> + int Bs[2] = {0,0}; >>>>>>>>> + T Bd[16]; >>>>>>>>> + >>>>>>>>> + for (j = 0; j < 4; j++) { >>>>>>>>> + for (i = 0; i + 1 <= j + 1; i++) { >>>>>>>>> + Ad[i + As[0] * j] = Bd[i + Bs[0] * j]; >>>>>>>>> + } >>>>>>>>> + >>>>>>>>> + i = j + 1; // <- comment out this line and it does not crash >>>>>>>>> + for (; i + 1 < 5; i++) { >>>>>>>>> + Ad[i + As[0] * j].a = 0.0; >>>>>>>>> + Ad[i + As[0] * j].b = 0.0; >>>>>>>>> + } >>>>>>>>> + } >>>>>>>>> +} >>>>>>>>> Index: gcc/tree-cfg.c >>>>>>>>> =================================================================== >>>>>>>>> --- 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); >>>>>>>>> >>>>>>>>> /* Various helpers. */ >>>>>>>>> @@ -2776,35 +2777,6 @@ last_and_only_stmt (basic_block bb) >>>>>>>>> return NULL; >>>>>>>>> } >>>>>>>>> >>>>>>>>> -/* 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<edge_var_map> *v = redirect_edge_var_map_vector (old_edge); >>>>>>>>> - if (!v) >>>>>>>>> - return; >>>>>>>>> - >>>>>>>>> - for (i = 0, phis = gsi_start_phis (new_edge->dest); >>>>>>>>> - v->iterate (i, &vm) && !gsi_end_p (phis); >>>>>>>>> - i++, gsi_next (&phis)) >>>>>>>>> - { >>>>>>>>> - gphi *phi = phis.phi (); >>>>>>>>> - tree result = redirect_edge_var_map_result (vm); >>>>>>>>> - tree arg = redirect_edge_var_map_def (vm); >>>>>>>>> - >>>>>>>>> - gcc_assert (result == 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 >>>>>>>>> block >>>>>>>>> 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; >>>>>>>>> } >>>>>>>>> >>>>>>>>> +/* 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 = ggc_cleared_alloc<edge_def> (); >>>>>>>>> + n_edges_for_fn (cfun)++; >>>>>>>>> + e->src = src; >>>>>>>>> + e->dest = dest; >>>>>>>>> + e->flags = flags; >>>>>>>>> + vec_safe_push (src->succs, e); >>>>>>>>> + e->dest_idx = e_in->dest_idx; >>>>>>>>> + return e; >>>>>>>>> +} >>>>>>>>> + >>>>>>>>> /* Split a (typically critical) edge EDGE_IN. Return the new block. >>>>>>>>> Abort on abnormal edges. */ >>>>>>>>> >>>>>>>>> @@ -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; >>>>>>>>> >>>>>>>>> /* Abnormal edges cannot be split. */ >>>>>>>>> gcc_assert (!(edge_in->flags & EDGE_ABNORMAL)); >>>>>>>>> @@ -2841,15 +2831,33 @@ gimple_split_edge (edge edge_in) >>>>>>>>> >>>>>>>>> after_bb = split_edge_bb_loc (edge_in); >>>>>>>>> >>>>>>>>> + /* Create a new block, and an edge F from that block to the >>>>>>>>> original >>>>>>>>> + destination. */ >>>>>>>>> new_bb = create_empty_bb (after_bb); >>>>>>>>> new_bb->frequency = EDGE_FREQUENCY (edge_in); >>>>>>>>> new_bb->count = edge_in->count; >>>>>>>>> - new_edge = make_single_succ_edge (new_bb, dest, EDGE_FALLTHRU); >>>>>>>>> + f = make_replacement_pred_edge (new_bb, dest, edge_in, >>>>>>>>> EDGE_FALLTHRU); >>>>>>>>> >>>>>>>>> - e = redirect_edge_and_branch (edge_in, new_bb); >>>>>>>>> + /* Redirect the original edge to its new successor. */ >>>>>>>>> + e = gimple_redirect_edge_and_branch_1 (edge_in, new_bb); >>>>>>>>> gcc_assert (e == edge_in); >>>>>>>>> - reinstall_phi_args (new_edge, e); >>>>>>>>> + e->dest = new_bb; >>>>>>>>> + vec_safe_push (new_bb->preds, e); >>>>>>>>> + e->dest_idx = 0; >>>>>>>>> >>>>>>>>> + /* 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 = -1; >>>>>>>>> + for (unsigned int i = 0; i < EDGE_COUNT (dest->preds); i++) >>>>>>>>> + if (EDGE_PRED (dest, i) == edge_in) >>>>>>>>> + { >>>>>>>>> + idx = i; >>>>>>>>> + break; >>>>>>>>> + } >>>>>>>>> + gcc_assert (idx != -1); >>>>>>>>> + EDGE_PRED (dest, idx) = f; >>>>>>>>> + >>>>>>>>> return new_bb; >>>>>>>>> } >>>>>>>>> >>>>>>>>> @@ -5740,12 +5748,10 @@ gimple_try_redirect_by_replacing_jump (edge >>>>>>>>> e, bas >>>>>>>>> return NULL; >>>>>>>>> } >>>>>>>>> >>>>>>>>> +/* Primary worker for gimple_redirect_edge_and_branch. */ >>>>>>>>> >>>>>>>>> -/* 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 = e->src; >>>>>>>>> gimple_stmt_iterator gsi; >>>>>>>>> @@ -5759,7 +5765,10 @@ static edge >>>>>>>>> return NULL; >>>>>>>>> >>>>>>>>> if (e->flags & EDGE_EH) >>>>>>>>> - return redirect_eh_edge (e, dest); >>>>>>>>> + { >>>>>>>>> + redirect_eh_edge_1 (e, dest, false); >>>>>>>>> + return e; >>>>>>>>> + } >>>>>>>>> >>>>>>>>> if (e->src != ENTRY_BLOCK_PTR_FOR_FN (cfun)) >>>>>>>>> { >>>>>>>>> @@ -5887,9 +5896,19 @@ static edge >>>>>>>>> gcc_assert (e->flags & EDGE_FALLTHRU); >>>>>>>>> break; >>>>>>>>> } >>>>>>>>> + return e; >>>>>>>>> +} >>>>>>>>> >>>>>>>>> - /* Update/insert PHI nodes as necessary. */ >>>>>>>>> +/* 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) >>>>>>>>> +{ >>>>>>>>> + edge f = gimple_redirect_edge_and_branch_1 (e, dest); >>>>>>>>> + if (f != e) >>>>>>>>> + return f; >>>>>>>>> + >>>>>>>>> /* Now update the edges in the CFG. */ >>>>>>>>> e = ssa_redirect_edge (e, dest); >>>>>>>>> >>>>>>>>> @@ -8636,13 +8655,18 @@ split_critical_edges (void) >>>>>>>>> basic_block bb; >>>>>>>>> edge e; >>>>>>>>> edge_iterator ei; >>>>>>>>> + int first_free_block; >>>>>>>>> >>>>>>>>> /* split_edge can redirect edges out of SWITCH_EXPRs, which can get >>>>>>>>> expensive. So we want to enable recording of edge to CASE_LABEL_EXPR >>>>>>>>> mappings around the calls to split_edge. */ >>>>>>>>> start_recording_case_labels (); >>>>>>>>> + first_free_block = last_basic_block_for_fn (cfun); >>>>>>>>> FOR_ALL_BB_FN (bb, cfun) >>>>>>>>> { >>>>>>>>> + /* Don't re-split edges we've already split. */ >>>>>>>>> + if (bb->index >= 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 >>>>>>>>> =================================================================== >>>>>>>>> --- 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. */ >>>>>>>>> >>>>>>>>> -static void >>>>>>>>> +void >>>>>>>>> redirect_eh_edge_1 (edge edge_in, basic_block new_bb, bool >>>>>>>>> change_region) >>>>>>>>> { >>>>>>>>> eh_landing_pad old_lp, new_lp; >>>>>>>>> Index: gcc/tree-eh.h >>>>>>>>> =================================================================== >>>>>>>>> --- 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_block); >>>>>>>>> extern bool operation_could_trap_helper_p (enum tree_code, bool, >>>>>>>>> bool, bool, >>>>>>>>> bool, tree, bool *);