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

["Kumar, Venkataramanan" <Venkataramanan dot Kumar at amd dot com>]

Hi Richard,


> -----Original Message-----
> From: Richard Biener [mailto:richard.guenther@gmail.com]
> Sent: Tuesday, August 04, 2015 4:07 PM
> To: Kumar, Venkataramanan
> Cc: Jeff Law; Jakub Jelinek; gcc-patches@gcc.gnu.org
> Subject: Re: [RFC] [Patch]: Try and vectorize with shift for mult expr with
> power 2 integer constant.
> 
> 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).
> 

I have updated your review comments in the attached patch. 

For the INT_MIN case, I am getting  vectorized output with the patch.   I believe x86_64 also vectorizes but does not negates the results. 

#include <limits.h>
unsigned long int  __attribute__ ((aligned (64)))arr[100];

int i;
#if 1
void test_vector_shifts()
{
        for(i=0; i<=99;i++)
        arr[i]=arr[i] * INT_MIN;
}
#endif

void test_vectorshift_via_mul()
{
        for(i=0; i<=99;i++)
        arr[i]=arr[i]*(-INT_MIN);

}

Before 
---------
        ldr     x1, [x0]
        neg     x1, x1, lsl 31
        str     x1, [x0], 8
        cmp     x0, x2

After 
-------
        ldr     q0, [x0]
        shl     v0.2d, v0.2d, 31
        neg     v0.2d, v0.2d
        str     q0, [x0], 16
        cmp     x1, x0

is this fine ?  

 > Thanks,
> Richard.
> 
> >>
> >>
> >>
> >> > @@ -2147,6 +2152,140 @@ vect_recog_vector_vector_shift_pattern
> >> (vec<gimple> *stmts,
> >> >     return pattern_stmt;
> >> >   }
> >> >
> >> > +/* Detect multiplication by constant which are postive or
> >> > +negatives of power 2,
> >> s/postive/positive/
> >>
> >>
> >> Jeff
> >
> > Regards,
> > Venkat.
> >

Attachment: vectorize_mults_via_shift_latest.diff.txt
Description: vectorize_mults_via_shift_latest.diff.txt