* [PATCH] Fix PR81354 (rewrite gimple_split_edge)
@ 2017-07-30 18:05 Bill Schmidt
2017-07-31 9:15 ` Richard Biener
0 siblings, 1 reply; 12+ messages in thread
From: Bill Schmidt @ 2017-07-30 18:05 UTC (permalink / raw)
To: GCC Patches, Richard Biener
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.
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 *);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-07-30 18:05 [PATCH] Fix PR81354 (rewrite gimple_split_edge) Bill Schmidt
@ 2017-07-31 9:15 ` Richard Biener
2017-07-31 13:19 ` Bill Schmidt
0 siblings, 1 reply; 12+ messages in thread
From: Richard Biener @ 2017-07-31 9:15 UTC (permalink / raw)
To: Bill Schmidt; +Cc: GCC Patches
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 *);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-07-31 9:15 ` Richard Biener
@ 2017-07-31 13:19 ` Bill Schmidt
2017-07-31 14:03 ` Bill Schmidt
0 siblings, 1 reply; 12+ messages in thread
From: Bill Schmidt @ 2017-07-31 13:19 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
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 *);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-07-31 13:19 ` Bill Schmidt
@ 2017-07-31 14:03 ` Bill Schmidt
2017-08-01 8:47 ` Richard Biener
0 siblings, 1 reply; 12+ messages in thread
From: Bill Schmidt @ 2017-07-31 14:03 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
> 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 *);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-07-31 14:03 ` Bill Schmidt
@ 2017-08-01 8:47 ` Richard Biener
2017-08-01 12:44 ` Bill Schmidt
0 siblings, 1 reply; 12+ messages in thread
From: Richard Biener @ 2017-08-01 8:47 UTC (permalink / raw)
To: Bill Schmidt; +Cc: GCC Patches
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 *);
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-08-01 8:47 ` Richard Biener
@ 2017-08-01 12:44 ` Bill Schmidt
2017-08-01 13:51 ` Bill Schmidt
0 siblings, 1 reply; 12+ messages in thread
From: Bill Schmidt @ 2017-08-01 12:44 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
> 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 *);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-08-01 12:44 ` Bill Schmidt
@ 2017-08-01 13:51 ` Bill Schmidt
2017-08-01 14:05 ` Richard Biener
2017-08-01 15:56 ` Bill Schmidt
0 siblings, 2 replies; 12+ messages in thread
From: Bill Schmidt @ 2017-08-01 13:51 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
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 *);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-08-01 13:51 ` Bill Schmidt
@ 2017-08-01 14:05 ` Richard Biener
2017-08-01 15:56 ` Bill Schmidt
1 sibling, 0 replies; 12+ messages in thread
From: Richard Biener @ 2017-08-01 14:05 UTC (permalink / raw)
To: Bill Schmidt; +Cc: GCC Patches
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 *);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-08-01 13:51 ` Bill Schmidt
2017-08-01 14:05 ` Richard Biener
@ 2017-08-01 15:56 ` Bill Schmidt
2017-08-02 8:09 ` Richard Biener
2017-08-04 17:52 ` Bill Schmidt
1 sibling, 2 replies; 12+ messages in thread
From: Bill Schmidt @ 2017-08-01 15:56 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
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 *);
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-08-01 15:56 ` Bill Schmidt
@ 2017-08-02 8:09 ` Richard Biener
2017-08-02 12:45 ` Bill Schmidt
2017-08-04 17:52 ` Bill Schmidt
1 sibling, 1 reply; 12+ messages in thread
From: Richard Biener @ 2017-08-02 8:09 UTC (permalink / raw)
To: Bill Schmidt; +Cc: GCC Patches
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 *);
>>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-08-02 8:09 ` Richard Biener
@ 2017-08-02 12:45 ` Bill Schmidt
0 siblings, 0 replies; 12+ messages in thread
From: Bill Schmidt @ 2017-08-02 12:45 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
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 *);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] Fix PR81354 (rewrite gimple_split_edge)
2017-08-01 15:56 ` Bill Schmidt
2017-08-02 8:09 ` Richard Biener
@ 2017-08-04 17:52 ` Bill Schmidt
1 sibling, 0 replies; 12+ messages in thread
From: Bill Schmidt @ 2017-08-04 17:52 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
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 *);
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-08-04 17:52 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-30 18:05 [PATCH] Fix PR81354 (rewrite gimple_split_edge) Bill Schmidt
2017-07-31 9:15 ` Richard Biener
2017-07-31 13:19 ` Bill Schmidt
2017-07-31 14:03 ` Bill Schmidt
2017-08-01 8:47 ` Richard Biener
2017-08-01 12:44 ` Bill Schmidt
2017-08-01 13:51 ` Bill Schmidt
2017-08-01 14:05 ` Richard Biener
2017-08-01 15:56 ` Bill Schmidt
2017-08-02 8:09 ` Richard Biener
2017-08-02 12:45 ` Bill Schmidt
2017-08-04 17:52 ` Bill Schmidt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).