Re: Fix twolf -funroll-loops -O3 miscompilation (a semi-latent web.c bug)

[Steven Bosscher <stevenb dot gcc at gmail dot com>]

On Fri, Oct 12, 2012 at 9:43 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
> Hi,
> I finally tracked down twolf misoptimization triggered by my loop-unroll.c
> changes.  It has turned out to be semi-latent wrong code issue in webizer.
> What happens is:
>
> 1) gcse.c drop REG_EQUAL note on the induction variable
> 2) loop optimizer unrolls the loop enabling webizer to cleanup
> 3) webizer do not track reaching refs into REG_EQUAL note because
>    the variable is dead and renames it to unused pseudo
>    Program is stil valid but the REG_EQUAL is bogus.

What do you mean, "not track"? web.c does track EQ notes via DF this:

web.c:315:  df_set_flags (DF_NO_HARD_REGS + DF_EQ_NOTES);



> 4) CSE eventually takes the value and put it back into the code
> 5) init-regs initializes it to 0
>
> and result is a segfault on the following testcase.
> Fixed by making webizer to not prune dead regs.
> Will commit it after testing on x86_64-linux.
>
> Honza
>
> /* { dg-do run } */
> /* { dg-options "-O3 -funroll-loops" } */
> typedef struct rowbox {
>     int startx ;
>     int endx ;
>     int endx1 ;
>     int startx2 ;
>     int ypos ;
>     int desiredL ;
> } ROWBOX ;
> ROWBOX rowArray1[2] ;
> ROWBOX *rowArray = rowArray1;
>
> int numRows = 2;
>
> int row = 1;
> int block = 0;
> double ckt_size_factor ;
>
> __attribute__ ((noinline))
> configure2()
> {
>   block = 0 ;
>   for( row = 1 ; row <= numRows ; row++ ) {
>       block++ ;
>     if( rowArray[row].endx1 > 0 ) {
>       block++ ;
>     }
>   }
> }
>
> main()
> {
>   configure2();
> }
>
>         * web.c (web_main): Do not set DF_RD_PRUNE_DEAD_DEFS flag.
> Index: web.c
> ===================================================================
> --- web.c       (revision 192369)
> +++ web.c       (working copy)
> @@ -313,7 +313,8 @@ web_main (void)
>    rtx insn;
>
>    df_set_flags (DF_NO_HARD_REGS + DF_EQ_NOTES);
> -  df_set_flags (DF_RD_PRUNE_DEAD_DEFS);
> +  /* We can not RD_PRUNE_DEAD_DEFS, because we care about REG_EQUAL
> +     notes.  */
>    df_chain_add_problem (DF_UD_CHAIN);
>    df_analyze ();
>    df_set_flags (DF_DEFER_INSN_RESCAN);

This is bogus. You're papering over the underlying bug of an invalid
REG_EQUAL note. This patch is not OK!

I did not add DF_RD_PRUNE_DEAD_DEFS for no reason. You're
re-introducing a major compile time hog. Did you do timings on some
significant body of code with -fweb enabled?

Ciao!
Steven