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

["Richard Guenther" <richard dot guenther at gmail dot com>]

On Mon, Apr 14, 2008 at 9:01 AM, Doug Kwan (Ãö®¶¼w) <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.