Fix PR81354 (rewrite gimple_split_edge)
diff mbox

Message ID CAFiYyc39SSRnNRUN+e0jKDcjo3B37LrD4VXPBww+QrxL13uERw@mail.gmail.com
State New
Headers show

Commit Message

Richard Biener July 31, 2017, 9:15 a.m. UTC
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.


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

Comments

Bill Schmidt July 31, 2017, 1:19 p.m. UTC | #1
That would certainly be much simpler!  I'll regstrap it and test it on the other
occurrence I've found to be certain.

-- 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 *);
Bill Schmidt July 31, 2017, 2:03 p.m. UTC | #2
> 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.

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 *);
>
Richard Biener Aug. 1, 2017, 8:46 a.m. UTC | #3
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.

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 *);
>>
>
Bill Schmidt Aug. 1, 2017, 12:44 p.m. UTC | #4
> 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.

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 *);
Bill Schmidt Aug. 1, 2017, 1:50 p.m. UTC | #5
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.

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 *);
Richard Biener Aug. 1, 2017, 2:05 p.m. UTC | #6
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 *);
>
Bill Schmidt Aug. 1, 2017, 3:56 p.m. UTC | #7
On Aug 1, 2017, at 8:50 AM, 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.

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.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 *);
>
Richard Biener Aug. 2, 2017, 8:09 a.m. UTC | #8
On Tue, Aug 1, 2017 at 5:56 PM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> On Aug 1, 2017, at 8:50 AM, 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.
>
> 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.

Richard.

> Bill
>
>>
>> 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 *);
>>
>
Bill Schmidt Aug. 2, 2017, 12:45 p.m. UTC | #9
On Aug 2, 2017, at 3:09 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Tue, Aug 1, 2017 at 5:56 PM, Bill Schmidt
> <wschmidt@linux.vnet.ibm.com> wrote:
>> On Aug 1, 2017, at 8:50 AM, 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.
>> 
>> 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.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 *);
Bill Schmidt Aug. 4, 2017, 5:52 p.m. UTC | #10
On Aug 1, 2017, at 10:56 AM, Bill Schmidt <wschmidt@linux.vnet.ibm.com> wrote:
> 
> On Aug 1, 2017, at 8:50 AM, 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.
> 
> 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.

Well, no.  I added the fix for 81503 to your patch, and I still get a failure on
trunk.  For the time being I am going to look at the edge-insertion idea 
before spending any more time on gimple_split_edge.

Bill

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

Patch
diff mbox

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;