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. 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 *); >
