This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)


On Tue, Aug 1, 2017 at 3:50 PM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> On Aug 1, 2017, at 7:44 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
>>
>>>
>>> On Aug 1, 2017, at 3:46 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>>
>>> On Mon, Jul 31, 2017 at 4:03 PM, Bill Schmidt
>>> <wschmidt@linux.vnet.ibm.com> wrote:
>>>>
>>>>> On Jul 31, 2017, at 8:19 AM, Bill Schmidt <wschmidt@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.

Btw you can likely also avoid the issue in SLSR by inserting on the edge
and doing commit_edge_insertions () at the end of the pass.

Richard.

> Bill
>
>>
>> Bill
>>>
>>> Richard.
>>>
>>>> Bill
>>>>
>>>>>
>>>>> -- Bill
>>>>>
>>>>>> On Jul 31, 2017, at 4:15 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>>>>>
>>>>>> On Sun, Jul 30, 2017 at 8:04 PM, Bill Schmidt
>>>>>> <wschmidt@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  <wschmidt@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  <wschmidt@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 *);
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]