[PATCH][TUPLES] Two simple bug fixes.

[PATCH][TUPLES] Two simple bug fixes.

[Doug Kwan (關振德)]

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

Re: [PATCH][TUPLES] Two simple bug fixes.

[Richard Guenther]

On Sun, Apr 13, 2008 at 9:35 AM, Doug Kwan (關振紱) <dougkwan@google.com> wrote:
> 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.

-ENOPATCH.

Richard.

>  -Doug
>

Fwd: [PATCH][TUPLES] Two simple bug fixes.

[Doug Kwan (關振德)]

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);

   /* 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
 >

Re: [PATCH][TUPLES] Two simple bug fixes.

[Richard Guenther]

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
>   >
>

Re: [PATCH][TUPLES] Two simple bug fixes.

[Doug Kwan (關振德)]

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?

-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
>  >   >
>  >
>

Re: [PATCH][TUPLES] Two simple bug fixes.

[Richard Guenther]

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
>  >  >   >
>  >  >
>  >
>

Re: [PATCH][TUPLES] Two simple bug fixes.

[Doug Kwan (關振德)]

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
>  >  >  >   >
>  >  >  >
>  >  >
>  >
>

Re: [PATCH][TUPLES] Two simple bug fixes.

[Richard Guenther]

On Mon, Apr 14, 2008 at 9:01 AM, Doug Kwan (關振德) <dougkwan@google.com> wrote:
> 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

/me looks into tuples branch.  We have

/* Return a new iterator initially pointing to GIMPLE_SEQ's last statement.  */

static inline gimple_stmt_iterator
gsi_last (gimple_seq seq)
{
  gimple_stmt_iterator i;

  i.ptr = gimple_seq_last (seq);
  i.seq = seq;
  i.bb = (i.ptr && i.ptr->stmt) ? gimple_bb (i.ptr->stmt) : NULL;

  return i;
}


/* Return a new iterator pointing to the last statement in basic block BB.  */

static inline gimple_stmt_iterator
gsi_last_bb (basic_block bb)
{
  gimple_stmt_iterator i;
  gimple_seq seq;

  seq = bb_seq (bb);
  i.ptr = gimple_seq_last (seq);
  i.seq = seq;
  i.bb = bb;

  return i;
}

why is the correct fix not removing this weird

  i.bb = (i.ptr && i.ptr->stmt) ? gimple_bb (i.ptr->stmt) : NULL;

casing and instead

  gcc_assert (i.ptr && i.ptr->stmt);
  i.bb = gimple_bb (i.ptr->stmt);

?

IMHO it's always invalid to create a gsi with a NULL BB.  Not?
Likewise for gsi_start.  Or if these are supposed to work before
CFG construction we should force the use of gsi_last_bb once
a CFG is constructed.

Or in gsi_insert_seq_nodes_before assert that if we have a CFG
that the gsi_bb is valid, not simply skip setting the bb for the stmts like

  if ((bb = gsi_bb (*i)) != NULL)
    update_bb_for_stmts (first, bb);

Thanks.
Richard.

Re: [PATCH][TUPLES] Two simple bug fixes.

[Doug Kwan (關振德)]

2008/4/14 Richard Guenther <richard.guenther@gmail.com>:

>  why is the correct fix not removing this weird
>
>   i.bb = (i.ptr && i.ptr->stmt) ? gimple_bb (i.ptr->stmt) : NULL;
>
>  casing and instead
>
>   gcc_assert (i.ptr && i.ptr->stmt);
>   i.bb = gimple_bb (i.ptr->stmt);
>
>  ?

If the seq is empty, i.ptr points to NULL, hence the special casing.

-Doug

Re: [PATCH][TUPLES] Two simple bug fixes.

[Doug Kwan (關振德)]

Sorry, I was wrong in my previous reply. i.ptr->stmt, not i.ptr is
NULL for an empty gimple_seq (I probably should go to bed by now :)).
Still, we need to special case.  I remember seeing a comment about
valid gsi with NULL bb when it points outside of any basic blocks.
However, I cannot find the relevant comment. (Diego, did you change
the API doc or did we ever allow a gsi with no bb?).

-Doug

Re: [PATCH][TUPLES] Two simple bug fixes.

[Richard Guenther]

On Mon, Apr 14, 2008 at 11:36 AM, Doug Kwan (關振紱) <dougkwan@google.com> wrote:
> Sorry, I was wrong in my previous reply. i.ptr->stmt, not i.ptr is
>  NULL for an empty gimple_seq (I probably should go to bed by now :)).
>  Still, we need to special case.  I remember seeing a comment about
>  valid gsi with NULL bb when it points outside of any basic blocks.
>  However, I cannot find the relevant comment. (Diego, did you change
>  the API doc or did we ever allow a gsi with no bb?).

Ok.  So we have bb in gsi only as

 1) cache if stmt is not NULL
 2) the bb for new stmts if the sequence is empty

somehow it feels "wrong" ;)  I realize that putting bb into gimple_seq
instead may equally feel wrong, but I often enough saw bogus stmt
bbs with

  bsi = bsi_for_stmt (stmt);
  split_block (bb_for_stmt (stmt), stmt);
  bsi_insert_before (&bsi, ...);

because split_block obviously doesn't update the bsi and while stmt
is still correct, the cached bb is not anymore.  The interface
bsi_for_stmt (stmt) doesn't suggest that the bb is cached but not updated.
So I end up doing

  bsi = bsi_for_stmt (bsi_stmt (bsi));

after splitting blocks, which looks ... ugly.

Maybe this sheds some light on my somewhat confusing queries to make
gsis less error prone here (and in the case you ran into, which is similar,
a bogus bb recorded in the gsi).

Thanks,
Richard.

Re: [PATCH][TUPLES] Two simple bug fixes.

[Diego Novillo]

On Sun, Apr 13, 2008 at 05:04, Richard Guenther
<richard.guenther@gmail.com> wrote:

>  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).

We can't really assert anything in gsi_last.  gsi_last_bb(bb) is
not semantically equivalent to gsi_last(bb_seq(bb)).

In gsi_last() a NULL sequence will simply return an iterator to
a NULL statement and associated to no basic block.  We cannot
really abort because there are sequences that are created and
manipulated before they are associated to a basic block.

OTOH, gsi_last_bb() will return an iterator to a NULL statement,
but associated to the basic block requested in the argument.
That's what makes it useful for empty blocks.

Doug, could you add a note explaining this difference in
gsi_last()/gsi_first()?


Thanks.  Diego.

Re: [PATCH][TUPLES] Two simple bug fixes.

[Doug Kwan (關振德)]

2008/4/14 Diego Novillo <dnovillo@google.com>:

>  Doug, could you add a note explaining this difference in
>  gsi_last()/gsi_first()?

Sure.

-Doug

Re: [PATCH][TUPLES] Two simple bug fixes.

[Doug Kwan (關振德)]

So are you guys okay with this fix ? If so, which version of the patch
should be checked in? The original one or the one with the assert?
The tree-ssa-sink patch depends on this bug fix.

-Doug

Re: [PATCH][TUPLES] Two simple bug fixes.

[Diego Novillo]

On 4/14/08 3:36 PM, Doug Kwan (???) wrote:
> So are you guys okay with this fix ? If so, which version of the patch
> should be checked in? The original one or the one with the assert?
> The tree-ssa-sink patch depends on this bug fix.

The assertion is not needed IMO but if Richard prefers it, I don't mind.


Diego.

Re: [PATCH][TUPLES] Two simple bug fixes.

[Richard Guenther]

On Mon, Apr 14, 2008 at 10:02 PM, Diego Novillo <dnovillo@google.com> wrote:
>
> On 4/14/08 3:36 PM, Doug Kwan (???) wrote:
>
> > So are you guys okay with this fix ? If so, which version of the patch
> > should be checked in? The original one or the one with the assert?
> > The tree-ssa-sink patch depends on this bug fix.
> >
>
>  The assertion is not needed IMO but if Richard prefers it, I don't mind.

Right, the assertion is not needed.  I was just fishing for some more
general robustness in the gsi_* routines themselves ;)

Richard.

Re: [PATCH][TUPLES] Two simple bug fixes.

[Doug Kwan (關振德)]

Too late. Assertion was checked in.  Hopefully gcc can figure out the
assertion is always true and eliminate it as dead code. :)

-Doug

2008/4/15 Richard Guenther <richard.guenther@gmail.com>:

>  Right, the assertion is not needed.  I was just fishing for some more
>  general robustness in the gsi_* routines themselves ;)
>
>  Richard.
>