| Message ID | CAFiYyc1wRxXfDjtKa97wtgTdPUQF9B1v3EWoLQWAAFOBFWfwCA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On 03/27/2017 08:27 AM, Richard Biener wrote: > On Mon, Mar 27, 2017 at 4:14 PM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Mon, Mar 27, 2017 at 2:47 PM, Martin Liška <mliska@suse.cz> wrote: >>> Hello. >>> >>> As described in the PR, we can create a PHI node in einline that has no argument. >>> That can cause ICE in devirtualization and should be thus handled. >>> >>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>> >>> Ready to be installed? >> >> We shouldn't ever have a PHI w/o argument. > > ;; basic block 14, loop depth 0 > ;; pred: > # SR.2_19 = PHI <> > <L4> [0.00%]: > a::~a (&g); > resx 12 > ;; succ: 17 > > the CFG has not been cleaned up here (this block is unreachable). > > Hmm, I see we are called from fold_stmt. I suppose we process > statements_to_fold before removing unreachable blocks to not > walk over dead stmts... chicken-and-egg... > > Still not creating that PHI should be possible. Can't speak for this specific example, but could it be the case that the PHI exists with arguments, then becomes unreachable resulting in a PHI with no arguments? jeff
On 03/27/2017 04:27 PM, Richard Biener wrote: > On Mon, Mar 27, 2017 at 4:14 PM, Richard Biener > <richard.guenther@gmail.com> wrote: >> On Mon, Mar 27, 2017 at 2:47 PM, Martin Liška <mliska@suse.cz> wrote: >>> Hello. >>> >>> As described in the PR, we can create a PHI node in einline that has no argument. >>> That can cause ICE in devirtualization and should be thus handled. >>> >>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>> >>> Ready to be installed? >> >> We shouldn't ever have a PHI w/o argument. > > ;; basic block 14, loop depth 0 > ;; pred: > # SR.2_19 = PHI <> > <L4> [0.00%]: > a::~a (&g); > resx 12 > ;; succ: 17 > > the CFG has not been cleaned up here (this block is unreachable). > > Hmm, I see we are called from fold_stmt. I suppose we process > statements_to_fold before removing unreachable blocks to not > walk over dead stmts... chicken-and-egg... > > Still not creating that PHI should be possible. I see, thanks for help. Patch fixes the issue, may I install it after regression tested? Thanks, Martin > > Index: gcc/tree-inline.c > =================================================================== > --- gcc/tree-inline.c (revision 246500) > +++ gcc/tree-inline.c (working copy) > @@ -2344,6 +2344,13 @@ copy_phis_for_bb (basic_block bb, copy_b > if (!virtual_operand_p (res)) > { > walk_tree (&new_res, copy_tree_body_r, id, NULL); > + if (EDGE_COUNT (new_bb->preds) == 0) > + { > + /* Technically we'd want a SSA_DEFAULT_DEF here... */ > + SSA_NAME_DEF_STMT (new_res) = gimple_build_nop (); > + } > + else > + { > new_phi = create_phi_node (new_res, new_bb); > FOR_EACH_EDGE (new_edge, ei, new_bb->preds) > { > @@ -2389,6 +2396,7 @@ copy_phis_for_bb (basic_block bb, copy_b > > add_phi_arg (new_phi, new_arg, new_edge, locus); > } > + } > } > } > > > >>> Martin
On Tue, Mar 28, 2017 at 9:45 AM, Martin Liška <mliska@suse.cz> wrote: > On 03/27/2017 04:27 PM, Richard Biener wrote: >> On Mon, Mar 27, 2017 at 4:14 PM, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> On Mon, Mar 27, 2017 at 2:47 PM, Martin Liška <mliska@suse.cz> wrote: >>>> Hello. >>>> >>>> As described in the PR, we can create a PHI node in einline that has no argument. >>>> That can cause ICE in devirtualization and should be thus handled. >>>> >>>> Patch can bootstrap on ppc64le-redhat-linux and survives regression tests. >>>> >>>> Ready to be installed? >>> >>> We shouldn't ever have a PHI w/o argument. >> >> ;; basic block 14, loop depth 0 >> ;; pred: >> # SR.2_19 = PHI <> >> <L4> [0.00%]: >> a::~a (&g); >> resx 12 >> ;; succ: 17 >> >> the CFG has not been cleaned up here (this block is unreachable). >> >> Hmm, I see we are called from fold_stmt. I suppose we process >> statements_to_fold before removing unreachable blocks to not >> walk over dead stmts... chicken-and-egg... >> >> Still not creating that PHI should be possible. > > I see, thanks for help. Patch fixes the issue, may I install it after > regression tested? I think it's progression, still not 100% safe as we have a SSA name with a GIMPLE_NOP definition that is not in any BB and the SSA name is not a default def. But yes, I've written a comment to that effect ;) Thus ok. An improvement would be to not copy unreachable regions at all... Thanks, Richard. > Thanks, > Martin > >> >> Index: gcc/tree-inline.c >> =================================================================== >> --- gcc/tree-inline.c (revision 246500) >> +++ gcc/tree-inline.c (working copy) >> @@ -2344,6 +2344,13 @@ copy_phis_for_bb (basic_block bb, copy_b >> if (!virtual_operand_p (res)) >> { >> walk_tree (&new_res, copy_tree_body_r, id, NULL); >> + if (EDGE_COUNT (new_bb->preds) == 0) >> + { >> + /* Technically we'd want a SSA_DEFAULT_DEF here... */ >> + SSA_NAME_DEF_STMT (new_res) = gimple_build_nop (); >> + } >> + else >> + { >> new_phi = create_phi_node (new_res, new_bb); >> FOR_EACH_EDGE (new_edge, ei, new_bb->preds) >> { >> @@ -2389,6 +2396,7 @@ copy_phis_for_bb (basic_block bb, copy_b >> >> add_phi_arg (new_phi, new_arg, new_edge, locus); >> } >> + } >> } >> } >> >> >> >>>> Martin >
Index: gcc/tree-inline.c =================================================================== --- gcc/tree-inline.c (revision 246500) +++ gcc/tree-inline.c (working copy) @@ -2344,6 +2344,13 @@ copy_phis_for_bb (basic_block bb, copy_b if (!virtual_operand_p (res)) { walk_tree (&new_res, copy_tree_body_r, id, NULL); + if (EDGE_COUNT (new_bb->preds) == 0) + { + /* Technically we'd want a SSA_DEFAULT_DEF here... */ + SSA_NAME_DEF_STMT (new_res) = gimple_build_nop (); + } + else + { new_phi = create_phi_node (new_res, new_bb); FOR_EACH_EDGE (new_edge, ei, new_bb->preds) { @@ -2389,6 +2396,7 @@ copy_phis_for_bb (basic_block bb, copy_b add_phi_arg (new_phi, new_arg, new_edge, locus); } + } } }