diff mbox

[i386,PR57623] Introduce _bextr (single underscore) intrinsucs

Message ID 51C43857.80507@gmail.com
State New
Headers show

Commit Message

Kirill Yukhin June 21, 2013, 11:26 a.m. UTC 
Hi,
As mentioned be Jakub, Intel Spec introduces single-underscored
intrinsics for bextr insn which takes different arguments.

Patch introduces intrinsics and tests.
ChangeLog:
2013-06-20  Kirill Yukhin 
<kirill.yukhin@intel.com>                                                                                              

                                                                                                                                                  

        * config/i386/i386-builtin-types.def:
Define                                                                                              

        UINT64_FTYPE_UINT64_UINT64_UINT64 and
UINT_FTYPE_UINT_UINT_UINT.                                                                          

        * config/i386/i386.c (IX86_BUILTIN_BEXTR32_3ARGS):
New.                                                                                   

        (IX86_BUILTIN_BEXTR64_3ARGS):
Ditto.                                                                                                      

        (bdesc_args): New builtins
definition.                                                                                                    

        (ix86_expand_args_builtin): Expand new ftypes. 

testsuite/ChangeLog:
2013-06-20  Kirill Yukhin 
<kirill.yukhin@intel.com>                                                                                              

                                                                                                                                                  

        * gcc.target/i386/bmi-1.c: Extend with new
instrinsic.                                                                                    

        Fix scan
patterns.                                                                                                                        

        * gcc.target/i386/bmi-1.c:
Ditto.                                                                                                         

        * gcc.target/i386/bmi-bextr-3.c:
New.                                                                                                     

        * gcc.target/i386/bmi-bextr-4.c:
Ditto.                                                                                                   


Is it ok to install?

Thanks, K
commit 2587fb012652f1c89f4cd4830a32e2e5c84bb88a (HEAD, refs/remotes/c/intel/kyukhin/bmi/bextr-intrin, refs/heads/intel/kyukhin/bmi/bextr-intrin)
Author: Kirill Yukhin <kirill.yukhin@intel.com>
Date:   Thu Jun 20 17:23:07 2013 +0400

    New intrinsics for bextr.

	Modified   gcc/ChangeLog

[back]

Comments

Uros Bizjak June 21, 2013, 3:11 p.m. UTC | #1 
On Fri, Jun 21, 2013 at 1:26 PM, Kirill Yukhin <kirill.yukhin@gmail.com> wrote:
> Hi,
> As mentioned be Jakub, Intel Spec introduces single-underscored
> intrinsics for bextr insn which takes different arguments.
>
> Patch introduces intrinsics and tests.
> ChangeLog:
> 2013-06-20  Kirill Yukhin
> <kirill.yukhin@intel.com>
>

Hm, your mailer is mangling lines.

>
>         * config/i386/i386-builtin-types.def:
> Define

Dot here.

Why do you need to change
>
>         UINT64_FTYPE_UINT64_UINT64_UINT64 and
> UINT_FTYPE_UINT_UINT_UINT.
>
>         * config/i386/i386.c (IX86_BUILTIN_BEXTR32_3ARGS):
> New.
>
>         (IX86_BUILTIN_BEXTR64_3ARGS):
> Ditto.
>
>         (bdesc_args): New builtins
> definition.
>
>         (ix86_expand_args_builtin): Expand new ftypes.
>
> testsuite/ChangeLog:
> 2013-06-20  Kirill Yukhin
> <kirill.yukhin@intel.com>
>
>
>
>         * gcc.target/i386/bmi-1.c: Extend with new
> instrinsic.
>
>         Fix scan
> patterns.
>
>         * gcc.target/i386/bmi-1.c:
> Ditto.
>
>         * gcc.target/i386/bmi-bextr-3.c:
> New.
>
>         * gcc.target/i386/bmi-bextr-4.c:
> Ditto.
>
>
> Is it ok to install?

This is OK for mainline.

BTW: There are many other single-underscore prefixed intrinsics [1]
besides _bextr_*. Perhaps you should also add these to the intrinsics
header, then the complete header could be backported to other release
branches.

[1] http://software.intel.com/sites/products/documentation/studio/composer/en-us/2011Update/compiler_c/intref_cls/common/intref_bk_avx2_manipulate.htm

Uros.
Jakub Jelinek June 21, 2013, 3:19 p.m. UTC | #2 
On Fri, Jun 21, 2013 at 05:11:39PM +0200, Uros Bizjak wrote:
> > Is it ok to install?
> 
> This is OK for mainline.
> 
> BTW: There are many other single-underscore prefixed intrinsics [1]
> besides _bextr_*. Perhaps you should also add these to the intrinsics
> header, then the complete header could be backported to other release
> branches.

I actually don't understand how this can work, bmi_bextr_{si,di} expanders
have just 3 operands (one target, 2 arguments), so just by giving it
4 operands instead just means the last one is dropped on the floor.
Why do you need a builtin for this at all?
I was expecting that _bextr_u{32,64} would be implemented either using
__bextr_u{32,64} or at least using it's underlying builtin, by constructing
the combined len/start argument with shifts/ands first.

But I admit I haven't applied the patch and looked how it works.

	Jakub
Uros Bizjak June 21, 2013, 3:28 p.m. UTC | #3 
On Fri, Jun 21, 2013 at 5:19 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Jun 21, 2013 at 05:11:39PM +0200, Uros Bizjak wrote:
>> > Is it ok to install?
>>
>> This is OK for mainline.
>>
>> BTW: There are many other single-underscore prefixed intrinsics [1]
>> besides _bextr_*. Perhaps you should also add these to the intrinsics
>> header, then the complete header could be backported to other release
>> branches.
>
> I actually don't understand how this can work, bmi_bextr_{si,di} expanders
> have just 3 operands (one target, 2 arguments), so just by giving it
> 4 operands instead just means the last one is dropped on the floor.
> Why do you need a builtin for this at all?
> I was expecting that _bextr_u{32,64} would be implemented either using
> __bextr_u{32,64} or at least using it's underlying builtin, by constructing
> the combined len/start argument with shifts/ands first.
>
> But I admit I haven't applied the patch and looked how it works.

OK, I assumed that the patch was tested according to established
standards, and that it doesn't need to be reviewed for its most basic
functionality. If the patch was not tested appropriately, I will
simply ignore future submissions from the submitters that want to bend
the rules. Sorry.

Uros.
diff mbox

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 53a6cde..b288f7c 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,12 @@ 
+2013-06-20  Kirill Yukhin  <kirill.yukhin@intel.com>
+
+	* config/i386/i386-builtin-types.def: Define
+	UINT64_FTYPE_UINT64_UINT64_UINT64 and UINT_FTYPE_UINT_UINT_UINT.
+	* config/i386/i386.c (IX86_BUILTIN_BEXTR32_3ARGS): New.
+	(IX86_BUILTIN_BEXTR64_3ARGS): Ditto.
+	(bdesc_args): New builtins definition.
+	(ix86_expand_args_builtin): Expand new ftypes.
+
 2013-06-19  Paolo Carlini  <paolo.carlini@oracle.com>
 
 	PR c++/56544
	Modified   gcc/config/i386/bmiintrin.h
diff --git a/gcc/config/i386/bmiintrin.h b/gcc/config/i386/bmiintrin.h
index 0087f5c..71965b3 100644
--- a/gcc/config/i386/bmiintrin.h
+++ b/gcc/config/i386/bmiintrin.h
@@ -52,6 +52,12 @@  __bextr_u32 (unsigned int __X, unsigned int __Y)
 }
 
 extern __inline unsigned int __attribute__((__gnu_inline__, __always_inline__, __artificial__))
+_bextr_u32 (unsigned int __X, unsigned int __Y, unsigned __Z)
+{
+  return __builtin_ia32_bextr_3args_u32 (__X, __Y, __Z);
+}
+
+extern __inline unsigned int __attribute__((__gnu_inline__, __always_inline__, __artificial__))
 __blsi_u32 (unsigned int __X)
 {
   return __X & -__X;
@@ -91,6 +97,12 @@  __bextr_u64 (unsigned long long __X, unsigned long long __Y)
 }
 
 extern __inline unsigned long long __attribute__((__gnu_inline__, __always_inline__, __artificial__))
+_bextr_u64 (unsigned long long __X, unsigned long long __Y, unsigned long long __Z)
+{
+  return __builtin_ia32_bextr_3args_u64 (__X, __Y, __Z);
+}
+
+extern __inline unsigned long long __attribute__((__gnu_inline__, __always_inline__, __artificial__))
 __blsi_u64 (unsigned long long __X)
 {
   return __X & -__X;
	Modified   gcc/config/i386/i386-builtin-types.def
diff --git a/gcc/config/i386/i386-builtin-types.def b/gcc/config/i386/i386-builtin-types.def
index 314f3e8..64620c8 100644
--- a/gcc/config/i386/i386-builtin-types.def
+++ b/gcc/config/i386/i386-builtin-types.def
@@ -377,6 +377,8 @@  DEF_FUNCTION_TYPE (VOID, UNSIGNED, UNSIGNED)
 DEF_FUNCTION_TYPE (INT, V16QI, V16QI, INT)
 DEF_FUNCTION_TYPE (UCHAR, UINT, UINT, UINT)
 DEF_FUNCTION_TYPE (UCHAR, UINT64, UINT, UINT)
+DEF_FUNCTION_TYPE (UINT, UINT, UINT, UINT)
+DEF_FUNCTION_TYPE (UINT64, UINT64, UINT64, UINT64)
 DEF_FUNCTION_TYPE (V16HI, V16HI, V16HI, V16HI)
 DEF_FUNCTION_TYPE (V16QI, V16QI, QI, INT)
 DEF_FUNCTION_TYPE (V16QI, V16QI, V16QI, INT)
	Modified   gcc/config/i386/i386.c
diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 0060b79..481f741 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -27034,6 +27034,8 @@  enum ix86_builtins
   /* BMI instructions.  */
   IX86_BUILTIN_BEXTR32,
   IX86_BUILTIN_BEXTR64,
+  IX86_BUILTIN_BEXTR32_3ARGS,
+  IX86_BUILTIN_BEXTR64_3ARGS,
   IX86_BUILTIN_CTZS,
 
   /* TBM instructions.  */
@@ -28213,7 +28215,9 @@  static const struct builtin_description bdesc_args[] =
 
   /* BMI */
   { OPTION_MASK_ISA_BMI, CODE_FOR_bmi_bextr_si, "__builtin_ia32_bextr_u32", IX86_BUILTIN_BEXTR32, UNKNOWN, (int) UINT_FTYPE_UINT_UINT },
+  { OPTION_MASK_ISA_BMI, CODE_FOR_bmi_bextr_si, "__builtin_ia32_bextr_3args_u32", IX86_BUILTIN_BEXTR32_3ARGS, UNKNOWN, (int) UINT_FTYPE_UINT_UINT_UINT },
   { OPTION_MASK_ISA_BMI, CODE_FOR_bmi_bextr_di, "__builtin_ia32_bextr_u64", IX86_BUILTIN_BEXTR64, UNKNOWN, (int) UINT64_FTYPE_UINT64_UINT64 },
+  { OPTION_MASK_ISA_BMI, CODE_FOR_bmi_bextr_di, "__builtin_ia32_bextr_3args_u64", IX86_BUILTIN_BEXTR64_3ARGS, UNKNOWN, (int) UINT64_FTYPE_UINT64_UINT64_UINT64 },
   { OPTION_MASK_ISA_BMI, CODE_FOR_ctzhi2,       "__builtin_ctzs",           IX86_BUILTIN_CTZS,    UNKNOWN, (int) UINT16_FTYPE_UINT16 },
 
   /* TBM */
@@ -31415,6 +31419,8 @@  ix86_expand_args_builtin (const struct builtin_description *d,
     case V4SF_FTYPE_V4SF_V4SF_V4SF:
     case V2DF_FTYPE_V2DF_V2DF_V2DF:
     case V32QI_FTYPE_V32QI_V32QI_V32QI:
+    case UINT64_FTYPE_UINT64_UINT64_UINT64:
+    case UINT_FTYPE_UINT_UINT_UINT:
       nargs = 3;
       break;
     case V32QI_FTYPE_V32QI_V32QI_INT:
	Modified   gcc/testsuite/ChangeLog
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index 1d7c2af..27ca5d2 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,11 @@ 
+2013-06-20  Kirill Yukhin  <kirill.yukhin@intel.com>
+
+	* gcc.target/i386/bmi-1.c: Extend with new instrinsic.
+	Fix scan patterns.
+	* gcc.target/i386/bmi-1.c: Ditto.
+	* gcc.target/i386/bmi-bextr-3.c: New.
+	* gcc.target/i386/bmi-bextr-4.c: Ditto.
+
 2013-06-19  Manuel Lopez-Ibanez  <manu@gcc.gnu.org>
 
 	PR c++/57638
	Modified   gcc/testsuite/gcc.target/i386/bmi-1.c
diff --git a/gcc/testsuite/gcc.target/i386/bmi-1.c b/gcc/testsuite/gcc.target/i386/bmi-1.c
index dc964ba..a05cb27 100644
--- a/gcc/testsuite/gcc.target/i386/bmi-1.c
+++ b/gcc/testsuite/gcc.target/i386/bmi-1.c
@@ -1,11 +1,11 @@ 
 /* { dg-do compile } */
 /* { dg-options "-O2 -mbmi " } */
-/* { dg-final { scan-assembler "andn\[^\\n]*(%|)eax" } } */
-/* { dg-final { scan-assembler "bextr\[^\\n]*(%|)eax" } } */
-/* { dg-final { scan-assembler "blsi\[^\\n]*(%|)eax" } } */
-/* { dg-final { scan-assembler "blsmsk\[^\\n]*(%|)eax" } } */
-/* { dg-final { scan-assembler "blsr\[^\\n]*(%|)eax" } } */
-/* { dg-final { scan-assembler "tzcntl\[^\\n]*(%|)eax" } } */
+/* { dg-final { scan-assembler "andn\[^\\n]*eax" } } */
+/* { dg-final { scan-assembler-times "bextr\[ \\t]+\[^\\n]*eax" 2 } } */
+/* { dg-final { scan-assembler "blsi\[^\\n]*eax" } } */
+/* { dg-final { scan-assembler "blsmsk\[^\\n]*eax" } } */
+/* { dg-final { scan-assembler "blsr\[^\\n]*eax" } } */
+/* { dg-final { scan-assembler "tzcntl\[^\\n]*eax" } } */
 
 #include <x86intrin.h>
 
@@ -22,6 +22,14 @@  func_bextr32 (unsigned int X, unsigned int Y)
 }
 
 unsigned int
+func_bextr32_3args (unsigned int X,
+		    unsigned int Y,
+		    unsigned int Z)
+{
+  return _bextr_u32(X, Y, Z);
+}
+
+unsigned int
 func_blsi32 (unsigned int X)
 {
   return __blsi_u32(X);
	Modified   gcc/testsuite/gcc.target/i386/bmi-2.c
diff --git a/gcc/testsuite/gcc.target/i386/bmi-2.c b/gcc/testsuite/gcc.target/i386/bmi-2.c
index 56f7387..68d06a2 100644
--- a/gcc/testsuite/gcc.target/i386/bmi-2.c
+++ b/gcc/testsuite/gcc.target/i386/bmi-2.c
@@ -1,11 +1,11 @@ 
 /* { dg-do compile { target { ! { ia32 }  } } } */
 /* { dg-options "-O2 -mbmi " } */
-/* { dg-final { scan-assembler "andn\[^\\n]*(%|)rax" } } */
-/* { dg-final { scan-assembler "bextr\[^\\n]*(%|)rax" } } */
-/* { dg-final { scan-assembler "blsi\[^\\n]*(%|)rax" } } */
-/* { dg-final { scan-assembler "blsmsk\[^\\n]*(%|)rax" } } */
-/* { dg-final { scan-assembler "blsr\[^\\n]*(%|)rax" } } */
-/* { dg-final { scan-assembler "tzcntq\[^\\n]*(%|)rax" } } */
+/* { dg-final { scan-assembler "andn\[^\\n]*rax" } } */
+/* { dg-final { scan-assembler-times "bextr\[ \\t]+\[^\\n]*rax" 2 } } */
+/* { dg-final { scan-assembler "blsi\[^\\n]*rax" } } */
+/* { dg-final { scan-assembler "blsmsk\[^\\n]*rax" } } */
+/* { dg-final { scan-assembler "blsr\[^\\n]*rax" } } */
+/* { dg-final { scan-assembler "tzcntq\[^\\n]*rax" } } */
 
 #include <x86intrin.h>
 
@@ -22,6 +22,14 @@  func_bextr64 (unsigned long long X, unsigned long long Y)
 }
 
 unsigned long long
+func_bextr64_3args (unsigned long long X,
+		    unsigned long long Y,
+		    unsigned long long Z)
+{
+  return _bextr_u64 (X, Y, Z);
+}
+
+unsigned long long
 func_blsi64 (unsigned long long X)
 {
   return __blsi_u64 (X);
	New        gcc/testsuite/gcc.target/i386/bmi-bextr-3.c
diff --git a/gcc/testsuite/gcc.target/i386/bmi-bextr-3.c b/gcc/testsuite/gcc.target/i386/bmi-bextr-3.c
new file mode 100644
index 0000000..004330a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/bmi-bextr-3.c
@@ -0,0 +1,47 @@ 
+/* { dg-do run { target { bmi && { ! ia32 } } } } */
+/* { dg-options "-O2 -mbmi -fno-inline" } */
+
+#include <x86intrin.h>
+
+#include "bmi-check.h"
+
+long long calc_bextr_u64 (unsigned long long src1,
+			  unsigned long long src2)
+{
+  long long res = 0;
+  unsigned char start = (src2 & 0xff);
+  unsigned char len = (int) ((src2 >> 8) & 0xff);
+  if (start < 64) {
+    unsigned i;
+    unsigned last = (start+len) < 64 ? start+len : 64;
+
+    src1 >>= start;
+    for (i=start; i<last; ++i) {
+      res |= (src1 & 1) << (i-start);
+      src1 >>= 1;
+    }
+  }
+
+  return res;
+}
+
+static void
+bmi_test ()
+{
+  unsigned i;
+  unsigned char start, len;
+  unsigned long long src1 = 0xfacec0ffeefacec0;
+  unsigned long long res, res_ref, src2;
+
+  for (i=0; i<5; ++i) {
+    start = (i * 1983) % 64;
+    len = i + (i * 1983) % 64;
+    src1 = src1 * 3;
+
+    res_ref = calc_bextr_u64 (src1, src2);
+    res = _bextr_u64 (src1, start, len);
+
+    if (res != res_ref)
+      abort();
+  }
+}
	New        gcc/testsuite/gcc.target/i386/bmi-bextr-4.c
diff --git a/gcc/testsuite/gcc.target/i386/bmi-bextr-4.c b/gcc/testsuite/gcc.target/i386/bmi-bextr-4.c
new file mode 100644
index 0000000..5a7767e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/bmi-bextr-4.c
@@ -0,0 +1,47 @@ 
+/* { dg-do run { target { bmi } } } */
+/* { dg-require-effective-target bmi  } */
+/* { dg-options "-O2 -mbmi -fno-inline" } */
+
+#include <x86intrin.h>
+
+#include "bmi-check.h"
+
+unsigned calc_bextr_u32 (unsigned src1, unsigned src2)
+{
+  unsigned res = 0;
+  unsigned char start = (src2 & 0xff);
+  unsigned char len = (int) ((src2 >> 8) & 0xff);
+  if (start < 32) {
+    unsigned i;
+    unsigned last = (start+len) < 32 ? start+len : 32;
+
+    src1 >>= start;
+    for (i=start; i<last; ++i) {
+      res |= (src1 & 1) << (i-start);
+      src1 >>= 1;
+    }
+  }
+
+  return res;
+}
+
+static void
+bmi_test ()
+{
+  unsigned i;
+  unsigned char start, len;
+  unsigned src1 = 0xfacec0ff;
+  unsigned res, res_ref, src2;
+
+  for (i=0; i<5; ++i) {
+    start = (i * 1983) % 32;
+    len = i + (i * 1983) % 32;
+    src1 = src1 * 3;
+
+    res_ref = calc_bextr_u32 (src1, src2);
+    res = _bextr_u32 (src1, start, len);
+
+    if (res != res_ref)
+      abort();
+  }
+}