Richard Biener <richard.guent...@gmail.com> writes:
> On Tue, Aug 4, 2015 at 4:21 PM, Richard Biener
> <richard.guent...@gmail.com> wrote:
>> On Tue, Aug 4, 2015 at 4:15 PM, Richard Sandiford
>> <richard.sandif...@arm.com> wrote:
>>> Richard Biener <richard.guent...@gmail.com> writes:
>>>> On Tue, Aug 4, 2015 at 10:52 AM, Kumar, Venkataramanan
>>>> <venkataramanan.ku...@amd.com> wrote:
>>>>> Hi Jeff,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
>>>>>> ow...@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.guent...@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.ku...@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.ku...@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
