Message ID | 20230504215638.988177-1-apinski@marvell.com |
---|---|
State | New |
Headers | show |
Series | PHIOPT: Fix diamond case of match_simplify_replacement | expand |
On Thu, May 4, 2023 at 11:57 PM Andrew Pinski via Gcc-patches <gcc-patches@gcc.gnu.org> wrote: > > So it turns out I messed checking which edge was true/false for the diamond > form. The edges, e0 and e1 here are edges from the merge block but the > true/false edges are from the conditional block and with diamond/threeway, > there is a bb inbetween on both edges. > Most of the time, the check that was in match_simplify_replacement would > happen to be correct for diamond form as most of the time the first edge in > the conditional is the edge for the true side of the conditional. > This is why I didn't see the issue during bootstrap/testing. > > I added a fragile gimple testcase which exposed the issue. Since there is > no way to specify the order of the edges in the gimple fe, we have to > have forwprop to swap the false/true edges (not order of them, just swapping > true/false flags) and hope not to do cleanupcfg inbetween forwprop and the > first phiopt pass. This is the fragile part really, it is not that we will > produce wrong code, just we won't hit what was the failing case. > > OK? Bootstrapped and tested on x86_64-linux-gnu. OK. Thanks, Richard. > PR tree-optimization/109732 > > gcc/ChangeLog: > > * tree-ssa-phiopt.cc (match_simplify_replacement): Fix the selection > of the argtrue/argfalse. > > gcc/testsuite/ChangeLog: > > * gcc.dg/pr109732.c: New test. > --- > gcc/testsuite/gcc.dg/pr109732.c | 40 +++++++++++++++++++++++++++++++++ > gcc/tree-ssa-phiopt.cc | 29 +++++++++++++++++++++--- > 2 files changed, 66 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/pr109732.c > > diff --git a/gcc/testsuite/gcc.dg/pr109732.c b/gcc/testsuite/gcc.dg/pr109732.c > new file mode 100644 > index 00000000000..d8374705cd8 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr109732.c > @@ -0,0 +1,40 @@ > +/* { dg-do run } */ > +/* We need to disable passes which might cause cfg cleanup */ > +/* { dg-options "-O1 -fgimple -fdisable-tree-ethread -fdisable-tree-fre1" } */ > + > +/* This code is done this way to have the false edge as 1st > + successor edge of BB2. Normally the true edge would be > + the first and you would not hit the bug. */ > +[[gnu::noipa]] > +_Bool __GIMPLE (ssa, startwith("forwprop1")) > +f3 (_Bool a) > +{ > + _Bool i; > + _Bool tt; > + > + __BB(2): > + tt_4 = a_1(D) == _Literal (_Bool)0; > + if (tt_4 != _Literal (_Bool)0) > + goto __BB3; > + else > + goto __BB4; > + > + __BB(3): > + goto __BB5; > + > + __BB(4): > + goto __BB5; > + > + __BB(5): > + i_2 = __PHI (__BB4: a_1(D), __BB3: _Literal (_Bool)0); > + > + return i_2; > +} > + > +int main() > +{ > + if (f3(0)) > + __builtin_abort(); > + if (!f3(1)) > + __builtin_abort(); > +} > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc > index 14aeaadd6f6..2fb28b4e60e 100644 > --- a/gcc/tree-ssa-phiopt.cc > +++ b/gcc/tree-ssa-phiopt.cc > @@ -726,6 +726,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, > gimple *stmt_to_move = NULL; > gimple *stmt_to_move_alt = NULL; > auto_bitmap inserted_exprs; > + tree arg_true, arg_false; > > /* Special case A ? B : B as this will always simplify to B. */ > if (operand_equal_for_phi_arg_p (arg0, arg1)) > @@ -756,12 +757,34 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, > /* We need to know which is the true edge and which is the false > edge so that we know when to invert the condition below. */ > extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge); > - if (e1 == true_edge || e0 == false_edge) > - std::swap (arg0, arg1); > + > + /* Forward the edges over the middle basic block. */ > + if (true_edge->dest == middle_bb) > + true_edge = EDGE_SUCC (true_edge->dest, 0); > + if (false_edge->dest == middle_bb) > + false_edge = EDGE_SUCC (false_edge->dest, 0); > + > + /* When THREEWAY_P then e1 will point to the edge of the final transition > + from middle-bb to end. */ > + if (true_edge == e0) > + { > + if (!threeway_p) > + gcc_assert (false_edge == e1); > + arg_true = arg0; > + arg_false = arg1; > + } > + else > + { > + gcc_assert (false_edge == e0); > + if (!threeway_p) > + gcc_assert (true_edge == e1); > + arg_true = arg1; > + arg_false = arg0; > + } > > tree type = TREE_TYPE (gimple_phi_result (phi)); > result = gimple_simplify_phiopt (early_p, type, stmt, > - arg0, arg1, > + arg_true, arg_false, > &seq); > if (!result) > return false; > -- > 2.39.1 >
diff --git a/gcc/testsuite/gcc.dg/pr109732.c b/gcc/testsuite/gcc.dg/pr109732.c new file mode 100644 index 00000000000..d8374705cd8 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr109732.c @@ -0,0 +1,40 @@ +/* { dg-do run } */ +/* We need to disable passes which might cause cfg cleanup */ +/* { dg-options "-O1 -fgimple -fdisable-tree-ethread -fdisable-tree-fre1" } */ + +/* This code is done this way to have the false edge as 1st + successor edge of BB2. Normally the true edge would be + the first and you would not hit the bug. */ +[[gnu::noipa]] +_Bool __GIMPLE (ssa, startwith("forwprop1")) +f3 (_Bool a) +{ + _Bool i; + _Bool tt; + + __BB(2): + tt_4 = a_1(D) == _Literal (_Bool)0; + if (tt_4 != _Literal (_Bool)0) + goto __BB3; + else + goto __BB4; + + __BB(3): + goto __BB5; + + __BB(4): + goto __BB5; + + __BB(5): + i_2 = __PHI (__BB4: a_1(D), __BB3: _Literal (_Bool)0); + + return i_2; +} + +int main() +{ + if (f3(0)) + __builtin_abort(); + if (!f3(1)) + __builtin_abort(); +} diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc index 14aeaadd6f6..2fb28b4e60e 100644 --- a/gcc/tree-ssa-phiopt.cc +++ b/gcc/tree-ssa-phiopt.cc @@ -726,6 +726,7 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, gimple *stmt_to_move = NULL; gimple *stmt_to_move_alt = NULL; auto_bitmap inserted_exprs; + tree arg_true, arg_false; /* Special case A ? B : B as this will always simplify to B. */ if (operand_equal_for_phi_arg_p (arg0, arg1)) @@ -756,12 +757,34 @@ match_simplify_replacement (basic_block cond_bb, basic_block middle_bb, /* We need to know which is the true edge and which is the false edge so that we know when to invert the condition below. */ extract_true_false_edges_from_block (cond_bb, &true_edge, &false_edge); - if (e1 == true_edge || e0 == false_edge) - std::swap (arg0, arg1); + + /* Forward the edges over the middle basic block. */ + if (true_edge->dest == middle_bb) + true_edge = EDGE_SUCC (true_edge->dest, 0); + if (false_edge->dest == middle_bb) + false_edge = EDGE_SUCC (false_edge->dest, 0); + + /* When THREEWAY_P then e1 will point to the edge of the final transition + from middle-bb to end. */ + if (true_edge == e0) + { + if (!threeway_p) + gcc_assert (false_edge == e1); + arg_true = arg0; + arg_false = arg1; + } + else + { + gcc_assert (false_edge == e0); + if (!threeway_p) + gcc_assert (true_edge == e1); + arg_true = arg1; + arg_false = arg0; + } tree type = TREE_TYPE (gimple_phi_result (phi)); result = gimple_simplify_phiopt (early_p, type, stmt, - arg0, arg1, + arg_true, arg_false, &seq); if (!result) return false;