[combine,RFC] Don't transform sign and zero extends inside mults

On 06/11/15 00:56, Segher Boessenkool wrote:
> On Thu, Nov 05, 2015 at 12:01:26PM +0000, Kyrill Tkachov wrote:
>> Thanks, that looks less intrusive. I did try it out on arm and aarch64.
>> It does work on the aarch64 testcase. However, there's also a correctness
>> regression, I'll try to explain inline....
>>> diff --git a/gcc/combine.c b/gcc/combine.c
>>> index c3db2e0..3bf7ffb 100644
>>> --- a/gcc/combine.c
>>> +++ b/gcc/combine.c
>>> @@ -5284,6 +5284,15 @@ subst (rtx x, rtx from, rtx to, int in_dest, int
>>> in_cond, int unique_copy)
>>>   	      || GET_CODE (SET_DEST (x)) == PC))
>>>   	fmt = "ie";
>>>   
>>> +      /* Substituting into the operands of a widening MULT is not likely
>>> +	 to create RTL matching a machine insn.  */
>>> +      if (code == MULT
>>> +	  && (GET_CODE (XEXP (x, 0)) == ZERO_EXTEND
>>> +	      || GET_CODE (XEXP (x, 0)) == SIGN_EXTEND)
>>> +	  && (GET_CODE (XEXP (x, 1)) == ZERO_EXTEND
>>> +	      || GET_CODE (XEXP (x, 1)) == SIGN_EXTEND))
>>> +	return x;
>> I think we should also add:
>>        && REG_P (XEXP (XEXP (x, 0), 0))
>>        && REG_P (XEXP (XEXP (x, 1), 0))
>>
>> to the condition. Otherwise I've seen regressions in the arm testsuite, the
>> gcc.target/arm/smlatb-1.s test in particular that tries to match the pattern
>> (define_insn "*maddhisi4tb"
>>    [(set (match_operand:SI 0 "s_register_operand" "=r")
>>      (plus:SI (mult:SI (ashiftrt:SI
>>                 (match_operand:SI 1 "s_register_operand" "r")
>>                 (const_int 16))
>>                (sign_extend:SI
>>                 (match_operand:HI 2 "s_register_operand" "r")))
>>           (match_operand:SI 3 "s_register_operand" "r")))]
>>
>>
>> There we have a sign_extend of a shift that we want to convert to the form
>> that the pattern expects. So adding the checks for REG_P fixes that for me.
> I'll have a look at this; I thought it should be handled with the new patch
> (attached), but maybe not.
>
>> For the correctness issue I saw on aarch64 the shortest case I could reduce
>> is:
>> short int a[16], b[16];
>> void
>> f5 (void)
>> {
>>    a[0] = b[0] / 14;
>> }
> (Without -mcpu=cortex-a53, or you just get a divide insn).
>
>> Is there a way that subst can signal some kind of "failed to substitute"
>> result?
> Yep, see new patch.  The "from == to" condition is for when subst is called
> just to simplify some code (normally with pc_rtx, pc_rtx).

Indeed, this looks better but it still needs the REG_P checks for the inner
operands of the extends to not screw up the arm case.
Here's my proposed extension. I've also modified the testcase slightly to not use
an unitialised variable. It still demonstrates the issue we're trying to solve.

Bootstrapped and tested on arm and aarch64.
I'll let you put it through it's paces on your setup :)

P.S. Do we want to restrict this to targets that have a widening mul optab like I did
in the original patch?

Thanks,
Kyrill

2015-11-06  Segher Boessenkool  <segher@kernel.crashing.org>
             Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * combine.c (subst): Don't substitute or simplify when
     handling register-wise widening multiply.
     (force_to_mode): Likewise.

2015-11-06  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.target/aarch64/umaddl_combine_1.c: New test.

>> If not, I got it to work by using that check to set the in_dest variable to
>> the
>> subsequent recursive call to subst, in a similar way to my original patch,
>> but I was
>> hoping to avoid overloading the meaning of in_dest.
> Yes me too :-)
>
>
> Segher
>
>
> diff --git a/gcc/combine.c b/gcc/combine.c
> index c3db2e0..c806db9 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -5284,6 +5284,20 @@ subst (rtx x, rtx from, rtx to, int in_dest, int in_cond, int unique_copy)
>   	      || GET_CODE (SET_DEST (x)) == PC))
>   	fmt = "ie";
>   
> +      /* Substituting into the operands of a widening MULT is not likely
> +	 to create RTL matching a machine insn.  */
> +      if (code == MULT
> +	  && (GET_CODE (XEXP (x, 0)) == ZERO_EXTEND
> +	      || GET_CODE (XEXP (x, 0)) == SIGN_EXTEND)
> +	  && (GET_CODE (XEXP (x, 1)) == ZERO_EXTEND
> +	      || GET_CODE (XEXP (x, 1)) == SIGN_EXTEND))
> +	{
> +	  if (from == to)
> +	    return x;
> +	  else
> +	    return gen_rtx_CLOBBER (GET_MODE (x), const0_rtx);
> +	}
> +
>         /* Get the mode of operand 0 in case X is now a SIGN_EXTEND of a
>   	 constant.  */
>         if (fmt[0] == 'e')
> @@ -8455,6 +8469,15 @@ force_to_mode (rtx x, machine_mode mode, unsigned HOST_WIDE_INT mask,
>         /* ... fall through ...  */
>   
>       case MULT:
> +      /* Substituting into the operands of a widening MULT is not likely to
> +	 create RTL matching a machine insn.  */
> +      if (code == MULT
> +	  && (GET_CODE (XEXP (x, 0)) == ZERO_EXTEND
> +	      || GET_CODE (XEXP (x, 0)) == SIGN_EXTEND)
> +	  && (GET_CODE (XEXP (x, 1)) == ZERO_EXTEND
> +	      || GET_CODE (XEXP (x, 1)) == SIGN_EXTEND))
> +	return gen_lowpart_or_truncate (mode, x);
> +
>         /* For PLUS, MINUS and MULT, we need any bits less significant than the
>   	 most significant bit in MASK since carries from those bits will
>   	 affect the bits we are interested in.  */

[combine,RFC] Don't transform sign and zero extends inside mults

Commit Message

Comments

Patch