[RFA,pr,target/60648] Fix non-canonical RTL from x86 backend -- P1 regression

On Wed, Mar 26, 2014 at 09:53:47PM +0000, Richard Sandiford wrote:
> Richard Henderson <rth@redhat.com> writes:
> > On 03/26/2014 12:40 PM, Jakub Jelinek wrote:
> >> On Wed, Mar 26, 2014 at 01:32:44PM -0600, Jeff Law wrote:
> >>> On 03/26/14 12:28, Jakub Jelinek wrote:
> >>>> (mult:SI (const_int 0) (const_int 4)) is IMHO far from being canonical.
> >>>> And, I'd say it is likely other target legitimization hooks would also try
> >>>> to simplify it similarly.
> >>>> simplify_gen_binary is used in several other places during expansion,
> >>>> so I don't see why it couldn't be desirable here.
> >>> No particular reason.  I'll try that since we disagree about the
> >>> validity of the RTL and we can both agree that using
> >>> simplify_gen_binary is reasonable.
> >> 
> >> Other possibility if you want to change it in the i386.c legitimize_address
> >> hook would be IMHO using force_reg instead of force_operand, it should be
> >> the same thing in most cases, except for these corner cases, and there would
> >> be no need to canonizalize anything afterwards.
> >> But, if the i?86 maintainers feel otherwise on this and think your patch is
> >> ok, I don't feel that strongly about this.
> >
> > I like this as a solution.  Let the combiner clean things up if it's
> > gotten so far.

Did you mean Jeff's original change, or say:
(or copy_to_reg, should be the same thing).

> How about doing both?  Jakub's simplify_gen_binary change looked like a good
> idea regardless of whatever else happens.  Seems a shame not to go with it.

Agreed.

	Jakub

On 03/27/2014 06:51 AM, Jakub Jelinek wrote:
> Did you mean Jeff's original change, or say:
> --- gcc/config/i386/i386.c	2014-03-20 17:41:45.917689676 +0100
> +++ gcc/config/i386/i386.c	2014-03-27 14:47:21.876254288 +0100
> @@ -13925,13 +13925,13 @@ ix86_legitimize_address (rtx x, rtx oldx
>        if (GET_CODE (XEXP (x, 0)) == MULT)
>  	{
>  	  changed = 1;
> -	  XEXP (x, 0) = force_operand (XEXP (x, 0), 0);
> +	  XEXP (x, 0) = copy_addr_to_reg (XEXP (x, 0));

I meant more like this.

>> How about doing both?  Jakub's simplify_gen_binary change looked like a good
>> idea regardless of whatever else happens.  Seems a shame not to go with it.
> 
> Agreed.

Certainly.


r~

On 03/27/14 07:51, Jakub Jelinek wrote:
> On Wed, Mar 26, 2014 at 09:53:47PM +0000, Richard Sandiford wrote:
>> Richard Henderson <rth@redhat.com> writes:
>>> On 03/26/2014 12:40 PM, Jakub Jelinek wrote:
>>>> On Wed, Mar 26, 2014 at 01:32:44PM -0600, Jeff Law wrote:
>>>>> On 03/26/14 12:28, Jakub Jelinek wrote:
>>>>>> (mult:SI (const_int 0) (const_int 4)) is IMHO far from being canonical.
>>>>>> And, I'd say it is likely other target legitimization hooks would also try
>>>>>> to simplify it similarly.
>>>>>> simplify_gen_binary is used in several other places during expansion,
>>>>>> so I don't see why it couldn't be desirable here.
>>>>> No particular reason.  I'll try that since we disagree about the
>>>>> validity of the RTL and we can both agree that using
>>>>> simplify_gen_binary is reasonable.
>>>>
>>>> Other possibility if you want to change it in the i386.c legitimize_address
>>>> hook would be IMHO using force_reg instead of force_operand, it should be
>>>> the same thing in most cases, except for these corner cases, and there would
>>>> be no need to canonizalize anything afterwards.
>>>> But, if the i?86 maintainers feel otherwise on this and think your patch is
>>>> ok, I don't feel that strongly about this.
>>>
>>> I like this as a solution.  Let the combiner clean things up if it's
>>> gotten so far.
>
> Did you mean Jeff's original change, or say:
> --- gcc/config/i386/i386.c	2014-03-20 17:41:45.917689676 +0100
> +++ gcc/config/i386/i386.c	2014-03-27 14:47:21.876254288 +0100
> @@ -13925,13 +13925,13 @@ ix86_legitimize_address (rtx x, rtx oldx
>         if (GET_CODE (XEXP (x, 0)) == MULT)
>   	{
>   	  changed = 1;
> -	  XEXP (x, 0) = force_operand (XEXP (x, 0), 0);
> +	  XEXP (x, 0) = copy_addr_to_reg (XEXP (x, 0));
>   	}
>
>         if (GET_CODE (XEXP (x, 1)) == MULT)
>   	{
>   	  changed = 1;
> -	  XEXP (x, 1) = force_operand (XEXP (x, 1), 0);
> +	  XEXP (x, 1) = copy_addr_to_reg (XEXP (x, 1));
>   	}
>
>         if (changed
> (or copy_to_reg, should be the same thing).
copy_addr_to_reg is probably better since it forces us into Pmode (which 
is useful if we had a mode-less constant).

Both the simplify_gen_binary change and the force_addr_to_reg change 
independently fix this problem.  I'll do a regression run with both of 
them installed for completeness.

jeff

On Thu, Mar 27, 2014 at 10:17:26AM -0600, Jeff Law wrote:
> >Did you mean Jeff's original change, or say:
> >--- gcc/config/i386/i386.c	2014-03-20 17:41:45.917689676 +0100
> >+++ gcc/config/i386/i386.c	2014-03-27 14:47:21.876254288 +0100
> >@@ -13925,13 +13925,13 @@ ix86_legitimize_address (rtx x, rtx oldx
> >        if (GET_CODE (XEXP (x, 0)) == MULT)
> >  	{
> >  	  changed = 1;
> >-	  XEXP (x, 0) = force_operand (XEXP (x, 0), 0);
> >+	  XEXP (x, 0) = copy_addr_to_reg (XEXP (x, 0));
> >  	}
> >
> >        if (GET_CODE (XEXP (x, 1)) == MULT)
> >  	{
> >  	  changed = 1;
> >-	  XEXP (x, 1) = force_operand (XEXP (x, 1), 0);
> >+	  XEXP (x, 1) = copy_addr_to_reg (XEXP (x, 1));
> >  	}
> >
> >        if (changed
> >(or copy_to_reg, should be the same thing).
> copy_addr_to_reg is probably better since it forces us into Pmode
> (which is useful if we had a mode-less constant).

Well, but in both of these cases you know that what you pass in is
a MULT and thus never mode-less.  That said, copy_addr_to_reg has the
advantage that it will ICE if the MULT isn't Pmode, but that really should
never happen for addresses, so not a big difference.

	Jakub