| Message ID | 5473C6BD.6080108@mentor.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 25, 2014 at 1:01 AM, Tom de Vries <Tom_deVries@mentor.com> wrote: > Richard, > > I ran into a problem with my oacc kernels directive patch series where > tail-merge added another entry into a region that was previously > single-entry-single-exit. > > That resulted in hitting this assert in calc_dfs_tree: > ... > /* This aborts e.g. when there is _no_ path from ENTRY to EXIT at all. */ > gcc_assert (di->nodes == (unsigned int) n_basic_blocks_for_fn (cfun) - 1); > ... > during a call to move_sese_region_to_fn. > > This patch makes sure that we abort earlier, with a clearer message of what > is actually wrong. > > Bootstrapped and reg-tested on x86_64. > > OK for trunk/stage3? I believe someone made the function work for SEME regions and I believe it is actually used to copy loops with multiple exits so I don't see how the patch can work in these cases? Thanks, Richard. > Thanks, > - Tom
On 25-11-14 10:28, Richard Biener wrote: > On Tue, Nov 25, 2014 at 1:01 AM, Tom de Vries <Tom_deVries@mentor.com> wrote: >> Richard, >> >> I ran into a problem with my oacc kernels directive patch series where >> tail-merge added another entry into a region that was previously >> single-entry-single-exit. >> >> That resulted in hitting this assert in calc_dfs_tree: >> ... >> /* This aborts e.g. when there is _no_ path from ENTRY to EXIT at all. */ >> gcc_assert (di->nodes == (unsigned int) n_basic_blocks_for_fn (cfun) - 1); >> ... >> during a call to move_sese_region_to_fn. >> >> This patch makes sure that we abort earlier, with a clearer message of what >> is actually wrong. >> >> Bootstrapped and reg-tested on x86_64. >> >> OK for trunk/stage3? > > I believe someone made the function work for SEME regions and I believe > it is actually used to copy loops with multiple exits This is the first part of the function comment for move_sese_region_to_fn: ... /* Move a single-entry, single-exit region delimited by ENTRY_BB and EXIT_BB to function DEST_CFUN. The whole region is replaced by a single basic block in the original CFG and the new basic block is returned. DEST_CFUN must not have a CFG yet. Note that the region need not be a pure SESE region. Blocks inside the region may contain calls to abort/exit. The only restriction is that ENTRY_BB should be the only entry point and it must dominate EXIT_BB. ... I'm guessing you're referring to the 'not pure SESE region' bit? So in fact, it's not a single-entry-single-exit region, but more a single-entry-at-most-one-continuation region. [ Note that in case of f.i. an eternal loop, we can also have single entry, no continuation. ] > so I don't see how the > patch can work in these cases? > The bbs with calls to abort/exit don't have any successor edges. verify_sese doesn't assert anything specific about suchs bbs. Thanks, - Tom
On Tue, Nov 25, 2014 at 3:59 PM, Tom de Vries <Tom_deVries@mentor.com> wrote: > On 25-11-14 10:28, Richard Biener wrote: >> >> On Tue, Nov 25, 2014 at 1:01 AM, Tom de Vries <Tom_deVries@mentor.com> >> wrote: >>> >>> Richard, >>> >>> I ran into a problem with my oacc kernels directive patch series where >>> tail-merge added another entry into a region that was previously >>> single-entry-single-exit. >>> >>> That resulted in hitting this assert in calc_dfs_tree: >>> ... >>> /* This aborts e.g. when there is _no_ path from ENTRY to EXIT at all. >>> */ >>> gcc_assert (di->nodes == (unsigned int) n_basic_blocks_for_fn (cfun) - >>> 1); >>> ... >>> during a call to move_sese_region_to_fn. >>> >>> This patch makes sure that we abort earlier, with a clearer message of >>> what >>> is actually wrong. >>> >>> Bootstrapped and reg-tested on x86_64. >>> >>> OK for trunk/stage3? >> >> >> I believe someone made the function work for SEME regions and I believe >> it is actually used to copy loops with multiple exits > > > This is the first part of the function comment for move_sese_region_to_fn: > ... > /* Move a single-entry, single-exit region delimited by ENTRY_BB and > EXIT_BB to function DEST_CFUN. The whole region is replaced by a > single basic block in the original CFG and the new basic block is > returned. DEST_CFUN must not have a CFG yet. > > Note that the region need not be a pure SESE region. Blocks inside > the region may contain calls to abort/exit. The only restriction > is that ENTRY_BB should be the only entry point and it must > dominate EXIT_BB. > ... > > I'm guessing you're referring to the 'not pure SESE region' bit? > > So in fact, it's not a single-entry-single-exit region, but more a > single-entry-at-most-one-continuation region. [ Note that in case of f.i. an > eternal loop, we can also have single entry, no continuation. ] > >> so I don't see how the >> patch can work in these cases? >> > > The bbs with calls to abort/exit don't have any successor edges. verify_sese > doesn't assert anything specific about suchs bbs. Ah, indeed. Patch is ok then. Thanks, Richard. > Thanks, > - Tom >
2014-11-23 Tom de Vries <tom@codesourcery.com> * tree-cfg.c (verify_sese): New function. (move_sese_region_to_fn): Call verify_sese. * tree-cfg.h (verify_sese): Declare. --- gcc/tree-cfg.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ gcc/tree-cfg.h | 1 + 2 files changed, 56 insertions(+) diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index e78554f..db9f6c2 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -6870,6 +6870,58 @@ fixup_loop_arrays_after_move (struct function *fn1, struct function *fn2, fixup_loop_arrays_after_move (fn1, fn2, loop); } +DEBUG_FUNCTION void +verify_sese (basic_block entry, basic_block exit, vec<basic_block> *bbs_p) +{ + basic_block bb; + edge_iterator ei; + edge e; + bitmap bbs = BITMAP_ALLOC (NULL); + int i; + + gcc_assert (entry != NULL); + gcc_assert (entry != exit); + gcc_assert (bbs_p != NULL); + + gcc_assert (bbs_p->length () > 0); + + FOR_EACH_VEC_ELT (*bbs_p, i, bb) + bitmap_set_bit (bbs, bb->index); + + gcc_assert (bitmap_bit_p (bbs, entry->index)); + gcc_assert (exit == NULL || bitmap_bit_p (bbs, exit->index)); + + FOR_EACH_VEC_ELT (*bbs_p, i, bb) + { + if (bb == entry) + { + gcc_assert (single_pred_p (entry)); + gcc_assert (!bitmap_bit_p (bbs, single_pred (entry)->index)); + } + else + for (ei = ei_start (bb->preds); !ei_end_p (ei); ei_next (&ei)) + { + e = ei_edge (ei); + gcc_assert (bitmap_bit_p (bbs, e->src->index)); + } + + if (bb == exit) + { + gcc_assert (single_succ_p (exit)); + gcc_assert (!bitmap_bit_p (bbs, single_succ (exit)->index)); + } + else + for (ei = ei_start (bb->succs); !ei_end_p (ei); ei_next (&ei)) + { + e = ei_edge (ei); + gcc_assert (bitmap_bit_p (bbs, e->dest->index)); + } + } + + BITMAP_FREE (bbs); +} + + /* Move a single-entry, single-exit region delimited by ENTRY_BB and EXIT_BB to function DEST_CFUN. The whole region is replaced by a single basic block in the original CFG and the new basic block is @@ -6918,6 +6970,9 @@ move_sese_region_to_fn (struct function *dest_cfun, basic_block entry_bb, bbs.create (0); bbs.safe_push (entry_bb); gather_blocks_in_sese_region (entry_bb, exit_bb, &bbs); +#ifdef ENABLE_CHECKING + verify_sese (entry_bb, exit_bb, &bbs); +#endif /* The blocks that used to be dominated by something in BBS will now be dominated by the new block. */ diff --git a/gcc/tree-cfg.h b/gcc/tree-cfg.h index 626e973..d35e5ba 100644 --- a/gcc/tree-cfg.h +++ b/gcc/tree-cfg.h @@ -73,6 +73,7 @@ extern bool gimple_duplicate_sese_tail (edge, edge, basic_block *, unsigned, basic_block *); extern void gather_blocks_in_sese_region (basic_block entry, basic_block exit, vec<basic_block> *bbs_p); +extern void verify_sese (basic_block, basic_block, vec<basic_block> *); extern basic_block move_sese_region_to_fn (struct function *, basic_block, basic_block, tree); extern void dump_function_to_file (tree, FILE *, int); -- 1.9.1