This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][TUPLES] Two simple bug fixes.
How about this patch?
Index: gcc/gcc/gimple-iterator.c
===================================================================
--- gcc/gcc/gimple-iterator.c (revision 134237)
+++ gcc/gcc/gimple-iterator.c (working copy)
@@ -566,7 +566,10 @@ gsi_move_before (gimple_stmt_iterator *f
void
gsi_move_to_bb_end (gimple_stmt_iterator *from, basic_block bb)
{
- gimple_stmt_iterator last = gsi_last (bb_seq (bb));
+ gimple_stmt_iterator last = gsi_last_bb (bb);
+#ifdef ENABLE_CHECKING
+ gcc_assert (gsi_bb (last) == bb);
+#endif
/* Have to check gsi_end_p because it could be an empty block. */
if (!gsi_end_p (last) && is_ctrl_stmt (gsi_stmt (last)))
Index: gcc/gcc/passes.c
===================================================================
--- gcc/gcc/passes.c (revision 134237)
+++ gcc/gcc/passes.c (working copy)
@@ -626,8 +626,8 @@ init_optimization_passes (void)
NEXT_PASS (pass_phi_only_cprop);
NEXT_PASS (pass_tree_ifcombine);
NEXT_PASS (pass_phiopt);
- NEXT_PASS (pass_tail_recursion);
#endif
+ NEXT_PASS (pass_tail_recursion);
NEXT_PASS (pass_ch);
#if 0
NEXT_PASS (pass_stdarg);
2008/4/13 Richard Guenther <richard.guenther@gmail.com>:
> On Sun, Apr 13, 2008 at 11:46 AM, Doug Kwan (關振德) <dougkwan@google.com> wrote:
> > We can add a check to make sure that the statement pointed to by the
> > gsi indeed have the bb set correctly but we already have a CFG
> > consistency check in the tuples branch and that's how this bug got
> > caught. Do we still need another check?
>
> Maybe I'm missing sth, but
>
>
> > > > - gimple_stmt_iterator last = gsi_last (bb_seq (bb));
> > > > + gimple_stmt_iterator last = gsi_last_bb (bb);
>
> this doesn't look CFG checking related. If the above change makes any
> difference
> then the returned gsi has to be different in some cases. Which is what I asked
> to check for in gsi_last. I don't know any of bb_seq or gsi_last or gsi_last_bb
> from their implementation, so I have no idea what the actual case was you ran
> into that made that difference - but it would be nice to catch it early.
>
> Richard.
>
>
>
> > -Doug
> >
> > 2008/4/13 Richard Guenther <richard.guenther@gmail.com>:
> >
> >
> > > 2008/4/13 Doug Kwan (關振德) <dougkwan@google.com>:
> > >
> > > > Here is the patch.
> > > >
> > > > -Doug
> > > >
> > > >
> > > >
> > > > 2008-04-13 Doug Kwan <dougkwan@google.com>
> > > >
> > > > * gimple-iterator.c (gsi_move_to_bb_end): Use gsi_last_bb
> > > > instead of calling both gsi_last and bb_seq.
> > > > * passes.c (init_optimization_passes): Re-eanble second tail-recursion
> > > > pass.
> > > >
> > > > Index: gcc/gcc/gimple-iterator.c
> > > > ===================================================================
> > > > --- gcc/gcc/gimple-iterator.c (revision 134208)
> > > > +++ gcc/gcc/gimple-iterator.c (working copy)
> > > > @@ -566,7 +566,7 @@ gsi_move_before (gimple_stmt_iterator *f
> > > > void
> > > > gsi_move_to_bb_end (gimple_stmt_iterator *from, basic_block bb)
> > > > {
> > > > - gimple_stmt_iterator last = gsi_last (bb_seq (bb));
> > > > + gimple_stmt_iterator last = gsi_last_bb (bb);
> > >
> > > Can we put an assert guarded by #if ENABLE_CHECKING for the case you ran
> > > into in gsi_last? I also hit similar issues if you split a bb when
> > > there are active
> > > tsis in that bb (they get invalid bbs and strange things occur).
> > >
> > > Otherwise this is ok.
> > >
> > > Richard.
> > >
> > >
> > >
> > > > /* Have to check gsi_end_p because it could be an empty block. */
> > > > if (!gsi_end_p (last) && is_ctrl_stmt (gsi_stmt (last)))
> > > > Index: gcc/gcc/passes.c
> > > > ===================================================================
> > > > --- gcc/gcc/passes.c (revision 134209)
> > > > +++ gcc/gcc/passes.c (working copy)
> > > > @@ -629,8 +629,8 @@ init_optimization_passes (void)
> > > > NEXT_PASS (pass_phi_only_cprop);
> > > > NEXT_PASS (pass_tree_ifcombine);
> > > > NEXT_PASS (pass_phiopt);
> > > > - NEXT_PASS (pass_tail_recursion);
> > > > #endif
> > > > + NEXT_PASS (pass_tail_recursion);
> > > > NEXT_PASS (pass_ch);
> > > > #if 0
> > > > NEXT_PASS (pass_stdarg);
> > > >
> > > >
> > > > 2008/4/13 Rafael Espindola <espindola@google.com>:
> > > >
> > > >
> > > > > Patch is missing.
> > > > >
> > > > > 2008/4/13 Doug Kwan (關振德) <dougkwan@google.com>:
> > > >
> > > >
> > > > >
> > > > >
> > > > > > Hi Diego,
> > > > > >
> > > > > > Could you please review this simple patch? The first fix is in
> > > > > > gsi_move_to_bb_end. The bug is quite subtle. In the old code, if
> > > > > > the target bb is empty, bb_seq will be empty and gsi_last will not
> > > > > > have the correct bb set. That causes the moved gimple to have NULL is
> > > > > > its bb. The second fix is to re-enable the second tail recursion
> > > > > > pass. I did not do so when I check in the patch for tree-tailcall.c.
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > > > -Doug
> > > > > >
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Rafael Avila de Espindola
> > > > >
> > > > > Google Ireland Ltd.
> > > > > Gordon House
> > > > > Barrow Street
> > > > > Dublin 4
> > > > > Ireland
> > > > >
> > > > > Registered in Dublin, Ireland
> > > > > Registration Number: 368047
> > > > >
> > > >
> > >
> >
>