Re: [PATCH] Add verify_sese
On Tue, Nov 25, 2014 at 3:59 PM, Tom de Vries wrote: > On 25-11-14 10:28, Richard Biener wrote: >> >> On Tue, Nov 25, 2014 at 1:01 AM, Tom de Vries >> 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 >
Re: [PATCH] Add verify_sese
On 25-11-14 10:28, Richard Biener wrote: On Tue, Nov 25, 2014 at 1:01 AM, Tom de Vries 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
Re: [PATCH] Add verify_sese
On Tue, Nov 25, 2014 at 1:01 AM, Tom de Vries 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
[PATCH] Add verify_sese
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?
Thanks,
- Tom
2014-11-23 Tom de Vries
* 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 *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 *bbs_p);
+extern void verify_sese (basic_block, basic_block, vec *);
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
