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