This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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

Gr.
Steven


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]