Re: [vtab and trunk] fix gcse for insn before note at end of BB

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

On 10/2/07, Steven Bosscher <stevenb.gcc@gmail.com> wrote:
> Alexandre Oliva wrote (xf.
> http://gcc.gnu.org/ml/gcc-patches/2007-10/msg00068.html):
>
> >       * gcse.c (hash_scan_set): Insert set in insn before note at
> >       the end of basic block.
> >
> > Index: gcc/gcse.c
> > ===================================================================
> > --- gcc/gcse.c.orig   2007-08-23 08:26:43.000000000 -0300
> > +++ gcc/gcse.c        2007-08-23 08:26:48.000000000 -0300
> > @@ -1760,8 +1760,8 @@ hash_scan_set (rtx pat, rtx insn, struct
> >                 modified.  Here we want to search from INSN+1 on, but
> >                 oprs_available_p searches from INSN on.  */
> >              && (insn == BB_END (BLOCK_FOR_INSN (insn))
> > -                || ((tmp = next_nonnote_insn (insn)) != NULL_RTX
> > -                    && oprs_available_p (pat, tmp))))
> > +                || (tmp = next_nonnote_insn (insn)) == NULL_RTX
> > +                || oprs_available_p (pat, tmp)))
> >       insert_set_in_table (pat, insn, table);
> >      }
> >    /* In case of store we want to consider the memory value as available in
>
>
> Actually, it seems to me that using next_nonnote_insn here is dangerous:
>
> Because next_nonnote_insn may run into BLOCK_FOR_INSN(insn)->next_bb,
> and tmp may be BB_HEAD (BLOCK_FOR_INSN(insn)->next_bb).
> oprs_available_p will use the INSN_UID(tmp) assuming that INSN_UID
> belongs to an insn contained in BLOCK_FOR_INSN(insn), which may not be
> the case thanks to next_nonnote_insn.
>
> I don't know if this can lead to odd failure cases (be it wrong code,
> missed optimizations, or whatever) but it looks wrong to me.
>
> (In fact, the middle end is in cfglayout mode when running GCSE, so
> BLOCK_FOR_INSN(insn)->next_bb may not have a control relation with
> BLOCK_FOR_INSN(insn), i.e. it does not have to be a successor of
> BLOCK_FOR_INSN(insn). Therefore, tmp could be basically insn anywhere
> in the current function.)
>
> FWIW This is a problem with all uses of {next,prev}_nonnote_insn. You
> probably should add some check  that BLOCK_FOR_INSN(tmp) ==
> BLOCK_FOR_INSN(insn), although avoiding running past
> BB_END(BLOCK_FOR_INSN(insn)) would be even better...

Indeed - we run into this problem before.  Perhaps specializing
next_nonnote_insn
for cfglayout mode is the way to go...  like

Index: emit-rtl.c
===================================================================
--- emit-rtl.c  (revision 128904)
+++ emit-rtl.c  (working copy)
@@ -2902,6 +2902,24 @@ previous_insn (rtx insn)
 rtx
 next_nonnote_insn (rtx insn)
 {
+  if (in_cfglayout)
+    {
+      basic_block bb;
+      if (!insn)
+       return insn;
+      bb = BLOCK_FOR_INSN (insn);
+      while (1)
+       {
+         insn = NEXT_INSN (insn);
+         if (!insn
+             || BLOCK_FOR_INSN (insn) != bb)
+           return NULL_RTX;
+
+         if (!NOTE_P (insn))
+           return insn;
+       }
+    }
+
   while (insn)
     {
       insn = NEXT_INSN (insn);

or similar.

Richard.