As there are some issues with abs() type conversions, I removed the related content from the patch but only kept the SSE2 support for abs(int).
For the define_expand I added as below, the else body is there to avoid fall-through transformations to ABS operation in optabs.c. Otherwise ABS will be converted to other operations even that we have corresponding instructions from SSSE3. (define_expand "abs<mode>2" [(set (match_operand:VI124_AVX2_48_AVX512F 0 "register_operand") (abs:VI124_AVX2_48_AVX512F (match_operand:VI124_AVX2_48_AVX512F 1 "nonimmediate_operand")))] "TARGET_SSE2" { if (!TARGET_SSSE3) ix86_expand_sse2_abs (operands[0], force_reg (<MODE>mode, operands[1])); else emit_insn (gen_rtx_SET (VOIDmode, operands[0], gen_rtx_ABS (<MODE>mode, operands[1]))); DONE; }) The patch is attached here. Please give me your comments. thanks, Cong diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 8a38316..84c7ab5 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,10 @@ +2013-10-22 Cong Hou <co...@google.com> + + PR target/58762 + * config/i386/i386-protos.h (ix86_expand_sse2_abs): New function. + * config/i386/i386.c (ix86_expand_sse2_abs): New function. + * config/i386/sse.md: Add SSE2 support to abs (8/16/32-bit-int). + 2013-10-14 David Malcolm <dmalc...@redhat.com> * dumpfile.h (gcc::dump_manager): New class, to hold state diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h index 3ab2f3a..ca31224 100644 --- a/gcc/config/i386/i386-protos.h +++ b/gcc/config/i386/i386-protos.h @@ -238,6 +238,7 @@ extern void ix86_expand_mul_widen_evenodd (rtx, rtx, rtx, bool, bool); extern void ix86_expand_mul_widen_hilo (rtx, rtx, rtx, bool, bool); extern void ix86_expand_sse2_mulv4si3 (rtx, rtx, rtx); extern void ix86_expand_sse2_mulvxdi3 (rtx, rtx, rtx); +extern void ix86_expand_sse2_abs (rtx, rtx); /* In i386-c.c */ extern void ix86_target_macros (void); diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 02cbbbd..71905fc 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -41696,6 +41696,53 @@ ix86_expand_sse2_mulvxdi3 (rtx op0, rtx op1, rtx op2) gen_rtx_MULT (mode, op1, op2)); } +void +ix86_expand_sse2_abs (rtx op0, rtx op1) +{ + enum machine_mode mode = GET_MODE (op0); + rtx tmp0, tmp1; + + switch (mode) + { + /* For 32-bit signed integer X, the best way to calculate the absolute + value of X is (((signed) X >> (W-1)) ^ X) - ((signed) X >> (W-1)). */ + case V4SImode: + tmp0 = expand_simple_binop (mode, ASHIFTRT, op1, + GEN_INT (GET_MODE_BITSIZE + (GET_MODE_INNER (mode)) - 1), + NULL, 0, OPTAB_DIRECT); + if (tmp0) + tmp1 = expand_simple_binop (mode, XOR, op1, tmp0, + NULL, 0, OPTAB_DIRECT); + if (tmp0 && tmp1) + expand_simple_binop (mode, MINUS, tmp1, tmp0, + op0, 0, OPTAB_DIRECT); + break; + + /* For 16-bit signed integer X, the best way to calculate the absolute + value of X is max (X, -X), as SSE2 provides the PMAXSW insn. */ + case V8HImode: + tmp0 = expand_unop (mode, neg_optab, op1, NULL_RTX, 0); + if (tmp0) + expand_simple_binop (mode, SMAX, op1, tmp0, op0, 0, + OPTAB_DIRECT); + break; + + /* For 8-bit signed integer X, the best way to calculate the absolute + value of X is min ((unsigned char) X, (unsigned char) (-X)), + as SSE2 provides the PMINUB insn. */ + case V16QImode: + tmp0 = expand_unop (mode, neg_optab, op1, NULL_RTX, 0); + if (tmp0) + expand_simple_binop (V16QImode, UMIN, op1, tmp0, op0, 0, + OPTAB_DIRECT); + break; + + default: + break; + } +} + /* Expand an insert into a vector register through pinsr insn. Return true if successful. */ diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md index c3f6c94..b85ded4 100644 --- a/gcc/config/i386/sse.md +++ b/gcc/config/i386/sse.md @@ -8721,7 +8721,7 @@ (set (attr "prefix_rex") (symbol_ref "x86_extended_reg_mentioned_p (insn)")) (set_attr "mode" "DI")]) -(define_insn "abs<mode>2" +(define_insn "*abs<mode>2" [(set (match_operand:VI124_AVX2_48_AVX512F 0 "register_operand" "=v") (abs:VI124_AVX2_48_AVX512F (match_operand:VI124_AVX2_48_AVX512F 1 "nonimmediate_operand" "vm")))] @@ -8733,6 +8733,20 @@ (set_attr "prefix" "maybe_vex") (set_attr "mode" "<sseinsnmode>")]) +(define_expand "abs<mode>2" + [(set (match_operand:VI124_AVX2_48_AVX512F 0 "register_operand") + (abs:VI124_AVX2_48_AVX512F + (match_operand:VI124_AVX2_48_AVX512F 1 "nonimmediate_operand")))] + "TARGET_SSE2" +{ + if (!TARGET_SSSE3) + ix86_expand_sse2_abs (operands[0], force_reg (<MODE>mode, operands[1])); + else + emit_insn (gen_rtx_SET (VOIDmode, operands[0], + gen_rtx_ABS (<MODE>mode, operands[1]))); + DONE; +}) + (define_insn "abs<mode>2" [(set (match_operand:MMXMODEI 0 "register_operand" "=y") (abs:MMXMODEI diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 075d071..cf5b942 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2013-10-22 Cong Hou <co...@google.com> + + PR target/58762 + * gcc.dg/vect/pr58762.c: New test. + 2013-10-14 Tobias Burnus <bur...@net-b.de> PR fortran/58658 diff --git a/gcc/testsuite/gcc.dg/vect/pr58762.c b/gcc/testsuite/gcc.dg/vect/pr58762.c new file mode 100644 index 0000000..8b114a7 --- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/pr58762.c @@ -0,0 +1,28 @@ +/* { dg-require-effective-target vect_int } */ +/* { dg-do compile } */ +/* { dg-options "-O2 -ftree-vectorize" } */ + +void test1 (char* a, char* b) +{ + int i; + for (i = 0; i < 10000; ++i) + a[i] = abs (b[i]); +} + +void test2 (short* a, short* b) +{ + int i; + for (i = 0; i < 10000; ++i) + a[i] = abs (b[i]); +} + +void test3 (int* a, int* b) +{ + int i; + for (i = 0; i < 10000; ++i) + a[i] = abs (b[i]); +} + +/* { dg-final { scan-tree-dump-times "vectorized 1 loops" 3 "vect" + { target i?86-*-* x86_64-*-* ia64-*-* } } } */ +/* { dg-final { cleanup-tree-dump "vect" } } */ On Thu, Oct 24, 2013 at 5:09 PM, Cong Hou <co...@google.com> wrote: > On Wed, Oct 23, 2013 at 11:18 PM, Jakub Jelinek <ja...@redhat.com> wrote: >> On Wed, Oct 23, 2013 at 09:40:21PM -0700, Cong Hou wrote: >>> On Wed, Oct 23, 2013 at 8:52 AM, Joseph S. Myers >>> <jos...@codesourcery.com> wrote: >>> > On Tue, 22 Oct 2013, Cong Hou wrote: >>> > >>> >> For abs(char/short), type conversions are needed as the current abs() >>> >> function/operation does not accept argument of char/short type. >>> >> Therefore when we want to get the absolute value of a char_val using >>> >> abs (char_val), it will be converted into abs ((int) char_val). It >>> >> then can be vectorized, but the generated code is not efficient as >>> >> lots of packings and unpackings are envolved. But if we convert >>> >> (char) abs ((int) char_val) to abs (char_val), the vectorizer will be >>> >> able to generate better code. Same for short. >>> > >>> > ABS_EXPR has undefined overflow behavior. Thus, abs ((int) -128) is >>> > defined (and we also define the subsequent conversion of +128 to signed >>> > char, which ISO C makes implementation-defined not undefined), and >>> > converting to an ABS_EXPR on char would wrongly make it undefined. For >>> > such a transformation to be valid (in the absence of VRP saying that -128 >>> > isn't a possible value) you'd need a GIMPLE representation for >>> > ABS_EXPR<overflow:wrap>, as distinct from ABS_EXPR<overflow:undefined>. >>> > You don't have the option there is for some arithmetic operations of >>> > converting to a corresponding operation on unsigned types. >>> > >>> >>> Yes, you are right. The method I use can guarantee wrapping on >>> overflow (either shift-xor-sub or max(x, -x)). Can I just add the >>> condition if (flag_wrapv) before the conversion I made to prevent the >>> undefined behavior on overflow? >> >> What HW insns you expand to is one thing, but if some GCC pass assumes that >> ABS_EXPR always returns non-negative value (many do, look e.g. at >> tree_unary_nonnegative_warnv_p, extract_range_from_unary_expr_1, >> simplify_const_relational_operation, etc., you'd need to grep for all >> ABS_EXPR/ABS occurrences) and optimizes code based on that fact, you get >> wrong code because (char) abs((char) -128) is well defined. >> If we change ABS_EXPR/ABS definition that it is well defined on the most >> negative value of the typ (resp. mode), then we loose all those >> optimizations, if we do that only for the char/short types, it would be >> quite weird, though we could keep the benefits, but at the RTL level we'd >> need to treat that way all the modes equal to short's mode and smaller (so, >> for sizeof(short) == sizeof(int) target even int's mode). > > > I checked those functions and they all consider the possibility of > overflow. For example, tree_unary_nonnegative_warnv_p only returns > true for ABS_EXPR on integers if overflow is undefined. If the > consequence of overflow is wrapping, I think converting (char) > abs((int)-128) to abs(-128) (-128 has char type) is safe. Can we do it > by checking flag_wrapv? > > I could also first remove the abs conversion content from this patch > but only keep the content of expanding abs() for i386. I will submit > it later. > > >> >> The other possibility is not to create the ABS_EXPRs of char/short anywhere, >> solve the vectorization issues either through tree-vect-patterns.c or >> as part of the vectorization type demotion/promotions, see the recent >> discussions for that, you'd represent the short/char abs for the vectorized >> loop say using the shift-xor-sub or builtin etc. and if you want to do the >> same thing for scalar code, you'd just have combiner try to match some >> sequence. > > > Yes, I could do it through tree-vect-patterns.c, if the abs conversion > is prohibited. Currently the only reason I need the abs conversion is > for vectorization. > > Vectorization type demotion/promotions is interesting, but I am afraid > we will face the same problem there. > > > Thank you for your comment! > > > Cong > > >> >> Jakub
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 8a38316..84c7ab5 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,10 @@ +2013-10-22 Cong Hou <co...@google.com> + + PR target/58762 + * config/i386/i386-protos.h (ix86_expand_sse2_abs): New function. + * config/i386/i386.c (ix86_expand_sse2_abs): New function. + * config/i386/sse.md: Add SSE2 support to abs (8/16/32-bit-int). + 2013-10-14 David Malcolm <dmalc...@redhat.com> * dumpfile.h (gcc::dump_manager): New class, to hold state diff --git a/gcc/config/i386/i386-protos.h b/gcc/config/i386/i386-protos.h index 3ab2f3a..ca31224 100644 --- a/gcc/config/i386/i386-protos.h +++ b/gcc/config/i386/i386-protos.h @@ -238,6 +238,7 @@ extern void ix86_expand_mul_widen_evenodd (rtx, rtx, rtx, bool, bool); extern void ix86_expand_mul_widen_hilo (rtx, rtx, rtx, bool, bool); extern void ix86_expand_sse2_mulv4si3 (rtx, rtx, rtx); extern void ix86_expand_sse2_mulvxdi3 (rtx, rtx, rtx); +extern void ix86_expand_sse2_abs (rtx, rtx); /* In i386-c.c */ extern void ix86_target_macros (void); diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c index 02cbbbd..71905fc 100644 --- a/gcc/config/i386/i386.c +++ b/gcc/config/i386/i386.c @@ -41696,6 +41696,53 @@ ix86_expand_sse2_mulvxdi3 (rtx op0, rtx op1, rtx op2) gen_rtx_MULT (mode, op1, op2)); } +void +ix86_expand_sse2_abs (rtx op0, rtx op1) +{ + enum machine_mode mode = GET_MODE (op0); + rtx tmp0, tmp1; + + switch (mode) + { + /* For 32-bit signed integer X, the best way to calculate the absolute + value of X is (((signed) X >> (W-1)) ^ X) - ((signed) X >> (W-1)). */ + case V4SImode: + tmp0 = expand_simple_binop (mode, ASHIFTRT, op1, + GEN_INT (GET_MODE_BITSIZE + (GET_MODE_INNER (mode)) - 1), + NULL, 0, OPTAB_DIRECT); + if (tmp0) + tmp1 = expand_simple_binop (mode, XOR, op1, tmp0, + NULL, 0, OPTAB_DIRECT); + if (tmp0 && tmp1) + expand_simple_binop (mode, MINUS, tmp1, tmp0, + op0, 0, OPTAB_DIRECT); + break; + + /* For 16-bit signed integer X, the best way to calculate the absolute + value of X is max (X, -X), as SSE2 provides the PMAXSW insn. */ + case V8HImode: + tmp0 = expand_unop (mode, neg_optab, op1, NULL_RTX, 0); + if (tmp0) + expand_simple_binop (mode, SMAX, op1, tmp0, op0, 0, + OPTAB_DIRECT); + break; + + /* For 8-bit signed integer X, the best way to calculate the absolute + value of X is min ((unsigned char) X, (unsigned char) (-X)), + as SSE2 provides the PMINUB insn. */ + case V16QImode: + tmp0 = expand_unop (mode, neg_optab, op1, NULL_RTX, 0); + if (tmp0) + expand_simple_binop (V16QImode, UMIN, op1, tmp0, op0, 0, + OPTAB_DIRECT); + break; + + default: + break; + } +} + /* Expand an insert into a vector register through pinsr insn. Return true if successful. */ diff --git a/gcc/config/i386/sse.md b/gcc/config/i386/sse.md index c3f6c94..b85ded4 100644 --- a/gcc/config/i386/sse.md +++ b/gcc/config/i386/sse.md @@ -8721,7 +8721,7 @@ (set (attr "prefix_rex") (symbol_ref "x86_extended_reg_mentioned_p (insn)")) (set_attr "mode" "DI")]) -(define_insn "abs<mode>2" +(define_insn "*abs<mode>2" [(set (match_operand:VI124_AVX2_48_AVX512F 0 "register_operand" "=v") (abs:VI124_AVX2_48_AVX512F (match_operand:VI124_AVX2_48_AVX512F 1 "nonimmediate_operand" "vm")))] @@ -8733,6 +8733,20 @@ (set_attr "prefix" "maybe_vex") (set_attr "mode" "<sseinsnmode>")]) +(define_expand "abs<mode>2" + [(set (match_operand:VI124_AVX2_48_AVX512F 0 "register_operand") + (abs:VI124_AVX2_48_AVX512F + (match_operand:VI124_AVX2_48_AVX512F 1 "nonimmediate_operand")))] + "TARGET_SSE2" +{ + if (!TARGET_SSSE3) + ix86_expand_sse2_abs (operands[0], force_reg (<MODE>mode, operands[1])); + else + emit_insn (gen_rtx_SET (VOIDmode, operands[0], + gen_rtx_ABS (<MODE>mode, operands[1]))); + DONE; +}) + (define_insn "abs<mode>2" [(set (match_operand:MMXMODEI 0 "register_operand" "=y") (abs:MMXMODEI diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog index 075d071..cf5b942 100644 --- a/gcc/testsuite/ChangeLog +++ b/gcc/testsuite/ChangeLog @@ -1,3 +1,8 @@ +2013-10-22 Cong Hou <co...@google.com> + + PR target/58762 + * gcc.dg/vect/pr58762.c: New test. + 2013-10-14 Tobias Burnus <bur...@net-b.de> PR fortran/58658 diff --git a/gcc/testsuite/gcc.dg/vect/pr58762.c b/gcc/testsuite/gcc.dg/vect/pr58762.c new file mode 100644 index 0000000..8b114a7 --- /dev/null +++ b/gcc/testsuite/gcc.dg/vect/pr58762.c @@ -0,0 +1,28 @@ +/* { dg-require-effective-target vect_int } */ +/* { dg-do compile } */ +/* { dg-options "-O2 -ftree-vectorize" } */ + +void test1 (char* a, char* b) +{ + int i; + for (i = 0; i < 10000; ++i) + a[i] = abs (b[i]); +} + +void test2 (short* a, short* b) +{ + int i; + for (i = 0; i < 10000; ++i) + a[i] = abs (b[i]); +} + +void test3 (int* a, int* b) +{ + int i; + for (i = 0; i < 10000; ++i) + a[i] = abs (b[i]); +} + +/* { dg-final { scan-tree-dump-times "vectorized 1 loops" 3 "vect" + { target i?86-*-* x86_64-*-* ia64-*-* } } } */ +/* { dg-final { cleanup-tree-dump "vect" } } */