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 August 4, 2015 4:28:26 PM GMT+02:00, Richard Sandiford <richard.sandiford@arm.com> wrote:
>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.

So you are saying exact_log2 is really exact_log2u?

>> Now re-sent with Richards company disclaimer stripped...
>
>Doh.  Sent via the right channels this time...
>
>Thanks,
>Richard