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.
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
