| Message ID | 20240301221437.1981406-1-ewlu@rivosinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | middle-end: Fix dominator information with loop duplication PR114197 | expand |
On Fri, Mar 1, 2024 at 11:16 PM Edwin Lu <ewlu@rivosinc.com> wrote: > > When adding the new_preheader to the cfg, only the new_preheader's dominator > information is updated. If one of the new basic block's children was part > of the original cfg and adding new_preheader to the cfg introduces another path > to that child, the child's dominator information will not be updated. This may > cause verify_dominator's assertion to fail. > > Force recalculating dominators for all duplicated basic blocks and their > successors when updating new_preheader's dominator information. We're already doing this (which IMO is bad), via the iterate_fix_dominators call. You're adding another bunch of similar things and I think the use of recompute_dominator isn't safe as it assumes all predecessors have valid dominator info. I'll have a look. Richard. > PR 114197 > > gcc/ChangeLog: > > * tree-vect-loop-manip.cc (slpeel_tree_duplicate_loop_to_edge_cfg): > Recalculate dominator info when adding new_preheader to cfg > > gcc/testsuite/ChangeLog: > > * gcc.dg/vect/pr114197.c: New test. > > Signed-off-by: Edwin Lu <ewlu@rivosinc.com> > --- > gcc/testsuite/gcc.dg/vect/pr114197.c | 18 ++++++++++++++++++ > gcc/tree-vect-loop-manip.cc | 17 ++++++++++++++++- > 2 files changed, 34 insertions(+), 1 deletion(-) > create mode 100644 gcc/testsuite/gcc.dg/vect/pr114197.c > > diff --git a/gcc/testsuite/gcc.dg/vect/pr114197.c b/gcc/testsuite/gcc.dg/vect/pr114197.c > new file mode 100644 > index 00000000000..b1fb807729c > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/vect/pr114197.c > @@ -0,0 +1,18 @@ > +/* { dg-do compile } */ > +/* { dg-additional-options "-O3" } */ > + > + > +#pragma pack(push) > +struct a { > + volatile signed b : 8; > +}; > +#pragma pack(pop) > +int c; > +static struct a d = {5}; > +void e() { > +f: > + for (c = 8; c < 55; ++c) > + if (!d.b) > + goto f; > +} > + > diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc > index f72da915103..0f3a489e78c 100644 > --- a/gcc/tree-vect-loop-manip.cc > +++ b/gcc/tree-vect-loop-manip.cc > @@ -1840,7 +1840,22 @@ slpeel_tree_duplicate_loop_to_edge_cfg (class loop *loop, edge loop_exit, > } > > if (was_imm_dom || duplicate_outer_loop) > - set_immediate_dominator (CDI_DOMINATORS, exit_dest, new_exit->src); > + { > + set_immediate_dominator (CDI_DOMINATORS, exit_dest, new_exit->src); > + > + /* Update the dominator info for children of duplicated bbs. */ > + for (unsigned i = 0; i < scalar_loop->num_nodes; i++) > + { > + basic_block dom_bb = NULL; > + edge e; > + edge_iterator ei; > + FOR_EACH_EDGE (e, ei, new_bbs[i]->succs) > + { > + dom_bb = recompute_dominator (CDI_DOMINATORS, e->dest); > + set_immediate_dominator (CDI_DOMINATORS, e->dest, dom_bb); > + } > + } > + } > > /* And remove the non-necessary forwarder again. Keep the other > one so we have a proper pre-header for the loop at the exit edge. */ > -- > 2.34.1 >
diff --git a/gcc/testsuite/gcc.dg/vect/pr114197.c b/gcc/testsuite/gcc.dg/vect/pr114197.c new file mode 100644 index 00000000000..b1fb807729c --- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/pr114197.c @@ -0,0 +1,18 @@ +/* { dg-do compile } */ +/* { dg-additional-options "-O3" } */ + + +#pragma pack(push) +struct a { + volatile signed b : 8; +}; +#pragma pack(pop) +int c; +static struct a d = {5}; +void e() { +f: + for (c = 8; c < 55; ++c) + if (!d.b) + goto f; +} + diff --git a/gcc/tree-vect-loop-manip.cc b/gcc/tree-vect-loop-manip.cc index f72da915103..0f3a489e78c 100644 --- a/gcc/tree-vect-loop-manip.cc +++ b/gcc/tree-vect-loop-manip.cc @@ -1840,7 +1840,22 @@ slpeel_tree_duplicate_loop_to_edge_cfg (class loop *loop, edge loop_exit, } if (was_imm_dom || duplicate_outer_loop) - set_immediate_dominator (CDI_DOMINATORS, exit_dest, new_exit->src); + { + set_immediate_dominator (CDI_DOMINATORS, exit_dest, new_exit->src); + + /* Update the dominator info for children of duplicated bbs. */ + for (unsigned i = 0; i < scalar_loop->num_nodes; i++) + { + basic_block dom_bb = NULL; + edge e; + edge_iterator ei; + FOR_EACH_EDGE (e, ei, new_bbs[i]->succs) + { + dom_bb = recompute_dominator (CDI_DOMINATORS, e->dest); + set_immediate_dominator (CDI_DOMINATORS, e->dest, dom_bb); + } + } + } /* And remove the non-necessary forwarder again. Keep the other one so we have a proper pre-header for the loop at the exit edge. */
When adding the new_preheader to the cfg, only the new_preheader's dominator information is updated. If one of the new basic block's children was part of the original cfg and adding new_preheader to the cfg introduces another path to that child, the child's dominator information will not be updated. This may cause verify_dominator's assertion to fail. Force recalculating dominators for all duplicated basic blocks and their successors when updating new_preheader's dominator information. PR 114197 gcc/ChangeLog: * tree-vect-loop-manip.cc (slpeel_tree_duplicate_loop_to_edge_cfg): Recalculate dominator info when adding new_preheader to cfg gcc/testsuite/ChangeLog: * gcc.dg/vect/pr114197.c: New test. Signed-off-by: Edwin Lu <ewlu@rivosinc.com> --- gcc/testsuite/gcc.dg/vect/pr114197.c | 18 ++++++++++++++++++ gcc/tree-vect-loop-manip.cc | 17 ++++++++++++++++- 2 files changed, 34 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.dg/vect/pr114197.c