Re: [RFC] [Patch]: Try and vectorize with shift for mult expr with power 2 integer constant.

[Richard Sandiford <richard dot sandiford at arm dot com>]

Richard Biener <richard.guenther@gmail.com> writes:
> On Tue, Aug 4, 2015 at 4:21 PM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Tue, Aug 4, 2015 at 4:15 PM, Richard Sandiford
>> <richard.sandiford@arm.com> wrote:
>>> Richard Biener <richard.guenther@gmail.com> writes:
>>>> On Tue, Aug 4, 2015 at 10:52 AM, Kumar, Venkataramanan
>>>> <Venkataramanan.Kumar@amd.com> wrote:
>>>>> Hi Jeff,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
>>>>>> owner@gcc.gnu.org] On Behalf Of Jeff Law
>>>>>> Sent: Monday, August 03, 2015 11:42 PM
>>>>>> To: Kumar, Venkataramanan; Jakub Jelinek
>>>>>> Cc: Richard Beiner (richard.guenther@gmail.com); gcc-patches@gcc.gnu.org
>>>>>> Subject: Re: [RFC] [Patch]: Try and vectorize with shift for mult
>>>>>> expr with
>>>>>> power 2 integer constant.
>>>>>>
>>>>>> On 08/02/2015 05:03 AM, Kumar, Venkataramanan wrote:
>>>>>> > Hi Jakub,
>>>>>> >
>>>>>> > Thank you for reviewing the patch.
>>>>>> >
>>>>>> > I have incorporated your comments in the attached patch.
>>>>>> Note Jakub is on PTO for the next 3 weeks.
>>>>>
>>>>>  Thank you for this information.
>>>>>
>>>>>>
>>>>>>
>>>>>> >
>>>>>> >
>>>>>> >
>>>>>> > vectorize_mults_via_shift.diff.txt
>>>>>> >
>>>>>> >
>>>>>> > diff --git a/gcc/testsuite/gcc.dg/vect/vect-mult-patterns.c
>>>>>> > b/gcc/testsuite/gcc.dg/vect/vect-mult-patterns.c
>>>>>> Jakub would probably like more testcases :-)
>>>>>>
>>>>>> The most obvious thing to test would be other shift factors.
>>>>>>
>>>>>> A negative test to verify we don't try to turn a multiply by
>>>>>> non-constant or
>>>>>> multiply by a constant that is not a power of 2 into shifts.
>>>>>
>>>>> I have added negative test in the attached patch.
>>>>>
>>>>>
>>>>>>
>>>>>> [ Would it make sense, for example, to turn a multiply by 3 into a
>>>>>> shift-add
>>>>>> sequence?  As Jakub said, choose_mult_variant can be your friend. ]
>>>>>
>>>>> Yes I will do that in a follow up patch.
>>>>>
>>>>> The new change log becomes
>>>>>
>>>>> gcc/ChangeLog
>>>>> 2015-08-04  Venkataramanan Kumar  <Venkataramanan.kumar@amd.com>
>>>>>      * tree-vect-patterns.c (vect_recog_mult_pattern): New function for vectorizing
>>>>>         multiplication patterns.
>>>>>      * tree-vectorizer.h: Adjust the number of patterns.
>>>>>
>>>>> gcc/testsuite/ChangeLog
>>>>> 2015-08-04  Venkataramanan Kumar  <Venkataramanan.kumar@amd.com>
>>>>>      * gcc.dg/vect/vect-mult-pattern-1.c: New
>>>>>     * gcc.dg/vect/vect-mult-pattern-2.c: New
>>>>>
>>>>> Bootstrapped and reg tested on aarch64-unknown-linux-gnu.
>>>>>
>>>>> Ok for trunk ?
>>>>
>>>> +  if (TREE_CODE (oprnd0) != SSA_NAME
>>>> +      || TREE_CODE (oprnd1) != INTEGER_CST
>>>> +      || TREE_CODE (itype) != INTEGER_TYPE
>>>>
>>>> INTEGRAL_TYPE_P (itype)
>>>>
>>>> +  optab = optab_for_tree_code (LSHIFT_EXPR, vectype, optab_vector);
>>>> +  if (!optab
>>>> +      || optab_handler (optab, TYPE_MODE (vectype)) == CODE_FOR_nothing)
>>>> +       return NULL;
>>>> +
>>>>
>>>> indent of the return stmt looks wrong
>>>>
>>>> +  /* Handle constant operands that are postive or negative powers of 2.  */
>>>> +  if ( wi::exact_log2 (oprnd1) != -1  ||
>>>> +       wi::exact_log2 (wi::neg (oprnd1)) != -1)
>>>>
>>>> no space after (, || goes to the next line.
>>>>
>>>> +    {
>>>> +      tree shift;
>>>> +
>>>> +      if (wi::exact_log2 (oprnd1) != -1)
>>>>
>>>> please cache wi::exact_log2
>>>>
>>>> in fact the first if () looks redundant if you simply put an else
>>>> return NULL
>>>> after a else if (wi::exact_log2 (wi::neg (oprnd1)) != -1)
>>>>
>>>> Note that the issue with INT_MIN is that wi::neg (INT_MIN) is INT_MIN
>>>> again, but it seems that wi::exact_log2 returns -1 in that case so you
>>>> are fine (and in fact not handling this case).
>>>
>>> Are you sure it returns -1 for INT_MIN?  It isn't supposed to, assuming
>>> INT_MIN is shorthand for "minimum value for a signed type".  wide_ints
>>> aren't signed, so INT_MIN is indistinguishable from an unsigned
>>> 1<<(prec-1).
>>
>> No, not sure.  I spotted
>>
>>   /* Reject cases where there are implicit -1 blocks above HIGH.  */
>>   if (x.len * HOST_BITS_PER_WIDE_INT < x.precision && x.sign_mask () < 0)
>>     return -1;
>>
>> and thought that would catch it.  I mean the tree value is negative so
>> exact_log2 must see it is a negative value.

That's handling the "compressed" format, e.g.:

  {1 << 63}

as a 64-bit short-hand for a 256-bit:

  {1 << 63,-1,-1,-1}

In this case more than one of the low x.precision bits are known to be set.

> Now re-sent with Richards company disclaimer stripped...

Doh.  Sent via the right channels this time...

Thanks,
Richard