Is eliminate_regs_in_insn allowed to generate invalid instruction?

Hi, Ian,
I think I found the cause. find_reloads_address returns 0 even
it reloads both parts of address (see the patch). Therefore, 
corresponding address_reloaded[i] is not set and in
the following code of find_reloads, 

   if (EXTRA_CONSTRAINT_STR (operand, c, p))
    win = 1;
/* If the address was already reloaded,
   we win as well.  */
   else if (MEM_P (operand)
	      && address_reloaded[i] == 1) <-- address_reloaded[i] still 0
     win = 1;
    ...

Extra reload is thus generated even it is unnecessary
and causes ICE. 

It looks like a general GCC bug. But I couldn't produce
 a test for x86. The following patch is bootstrapped 
and passes tests on x86_64-unknown-linux-gnu. Is it
 OK to apply the patch?

Cheers,
Bingfeng 



> -----Original Message-----

> From: Ian Lance Taylor [mailto:iant@google.com]

> Sent: 17 December 2010 15:10

> To: Bingfeng Mei

> Cc: gcc@gcc.gnu.org

> Subject: Re: Is eliminate_regs_in_insn allowed to generate invalid

> instruction?

> 

> "Bingfeng Mei" <bmei@broadcom.com> writes:

> 

> > from gen_reload function.

> >   /* Otherwise, just write (set OUT IN) and hope for the best.  */

> >   else

> >     emit_insn (gen_rtx_SET (VOIDmode, out, in));

> 

> Those lines are one of the curses of reload.  When you hit them, you

> know something has gone wrong.  Unfortunately, exactly what has gone

> wrong can be difficult to determine.  It basically means that you are

> trying to reload something that the reload pass does not understand.

> 

> 

> > The comment doesn’t sound very convincing to me.

> >

> >

> > From debug message:

> > Reloads for insn # 680

> > Reload 0: reload_in (SI) = (plus:SI (reg/f:SI 57 r57)

> >                                                     (const_int 40

> [0x28]))

> > 	GR_REGS, RELOAD_FOR_INPUT_ADDRESS (opnum = 1)

> > 	reload_in_reg: (plus:SI (reg/f:SI 57 r57)

> >                                                     (const_int 40

> [0x28]))

> > 	reload_reg_rtx: (reg:SI 4 r4)

> > Reload 1: reload_in (SI) = (plus:SI (reg/f:SI 57 r57)

> >                                                     (const_int 78396

> [0x1323c]))

> > 	GR_REGS, RELOAD_FOR_INPUT_ADDRESS (opnum = 1), can't combine

> > 	reload_in_reg: (plus:SI (reg/f:SI 57 r57)

> >                                                     (const_int 78396

> [0x1323c]))

> > 	reload_reg_rtx: (reg:SI 6 r6)

> > Reload 2: reload_in (SI) = (mem/c:SI (plus:SI (reg/f:SI 57 r57)

> >                                                         (const_int

> 78396 [0x1323c])) [50 %sfp+78252 S4 A32])

> > 	GR_REGS, RELOAD_FOR_INPUT_ADDRESS (opnum = 1), can't combine

> > 	reload_in_reg: (reg:SI 596 [ ivtmp.474 ])

> > 	reload_reg_rtx: (reg:SI 9 r9)

> > Reload 3: reload_in (SI) = (plus:SI (mult:SI (reg:SI 596

> [ ivtmp.474 ])

> >                                                         (const_int 8

> [0x8]))

> >                                                     (plus:SI

> (reg/f:SI 57 r57)

> >                                                         (const_int 40

> [0x28])))

> > 	GR_REGS, RELOAD_FOR_INPUT (opnum = 1), inc by 8

> > 	reload_in_reg: (plus:SI (mult:SI (reg:SI 596 [ ivtmp.474 ])

> >                                                         (const_int 8

> [0x8]))

> >                                                     (plus:SI

> (reg/f:SI 57 r57)

> >                                                         (const_int 40

> [0x28])))

> > 	reload_reg_rtx: (reg:SI 4 r4)

> >

> >

> > I don’t understand why Reload 3 is necessary. After reload 0, 1, 2,

> > The following expression already fits into our addressing mode.

> >

> > (plus:SI (mult:SI (reg:SI 9 r9)  (const_int 8 [0x8]))

> >                                  (reg:SI 4 r4))

> >

> > Instead GCC tries to generate invalid

> > (insn 1716 1715 680 84 src/weighted_prediction.c:729 (set (reg:SI 4

> r4)

> >         (plus:SI (mult:SI (reg:SI 9 r9)

> >                 (const_int 8 [0x8]))

> >             (reg:SI 4 r4))) -1 (nil))

> > and load from [r4] subsequently.

> 

> Sounds like you're well on the way to tracking down the problem: you

> need to see why gcc decided to add reload 3 given the existence of the

> reloads 0 through 2.  In particular, look at the code after

>   if (strict_memory_address_addr_space_p (mode, ad, as))

> in find_reloads_address.

> 

> Ian

"Bingfeng Mei" <bmei@broadcom.com> writes:

> I think I found the cause. find_reloads_address returns 0 even
> it reloads both parts of address (see the patch). Therefore, 
> corresponding address_reloaded[i] is not set and in
> the following code of find_reloads, 
>
>    if (EXTRA_CONSTRAINT_STR (operand, c, p))
>     win = 1;
> /* If the address was already reloaded,
>    we win as well.  */
>    else if (MEM_P (operand)
> 	      && address_reloaded[i] == 1) <-- address_reloaded[i] still 0
>      win = 1;
>     ...
>
> Extra reload is thus generated even it is unnecessary
> and causes ICE. 
>
> It looks like a general GCC bug. But I couldn't produce
>  a test for x86. The following patch is bootstrapped 
> and passes tests on x86_64-unknown-linux-gnu. Is it
>  OK to apply the patch?
>
> Cheers,
> Bingfeng 
>
>
> Index: reload.c
> ===================================================================
> --- reload.c    (revision 167979)
> +++ reload.c    (working copy)
> @@ -5188,7 +5188,7 @@ find_reloads_address (enum machine_mode
>                                   &XEXP (ad, 1 - op_index), opnum,
>                                   type, 0, insn);
>
> -         return 0;
> +         return 1;

I'm willing to believe that there is a problem here, but that patch
isn't right.  find_reloads_address should only return 1 if the address
is replaced or reloaded as a whole, and that is not what is happening
here.  What is happening here is that the components of the address are
being reloaded.

Frankly I would fix this problem in LEGITIMIZE_RELOAD_ADDRESS.

Ian