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

[Richard Biener <richard dot guenther at gmail dot com>]

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.

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

> Richard.
>
>> wi::exact_log2 (wi::to_widest (INT_MIN)) would return -1, but that's
>> the difference between "infinite" precision and exact precision.
>>
>> Thanks,
>> Richard