Patchwork Fix up bmi_bextr_<mode> (PR target/57623)

login
register
mail settings
Submitter Jakub Jelinek
Date June 17, 2013, 4:11 p.m.
Message ID <20130617161152.GZ2336@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/251913/
State New
Headers show

Comments

Jakub Jelinek - June 17, 2013, 4:11 p.m.
Hi!

This instruction has the predicates/constraints wrong, the r/m argument is
the middle-one, the value from which it should be extracted, rather than
the packed start/length argument.  This got broken with PR50766, where the
patch hasn't touched just BMI2, but for unknown reasons also this BMI
instruction which was handled right in GCC 4.6.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8/4.7?

BTW, the AVX2 docs document _bextr_u{32,64} 3 argument intrinsics, rather
than the __bextr_u{32,64} 2 argument intrinsics we have in GCC right now.
Something to fix up (as the names are different, perhaps we can both the old
ones and the new ones implemented say as return __bextr_u32 (x, (y & 255) |
(z << 8)); or similar)?

2013-06-17  Jakub Jelinek  <jakub@redhat.com>

	PR target/57623
	* config/i386/i386.md (bmi_bextr_<mode>): Swap predicates and
	constraints of operand 1 and 2.

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


	Jakub
Uros Bizjak - June 18, 2013, 9:47 a.m.
On Mon, Jun 17, 2013 at 6:11 PM, Jakub Jelinek <jakub@redhat.com> wrote:

> This instruction has the predicates/constraints wrong, the r/m argument is
> the middle-one, the value from which it should be extracted, rather than
> the packed start/length argument.  This got broken with PR50766, where the
> patch hasn't touched just BMI2, but for unknown reasons also this BMI
> instruction which was handled right in GCC 4.6.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8/4.7?
>
> BTW, the AVX2 docs document _bextr_u{32,64} 3 argument intrinsics, rather
> than the __bextr_u{32,64} 2 argument intrinsics we have in GCC right now.
> Something to fix up (as the names are different, perhaps we can both the old
> ones and the new ones implemented say as return __bextr_u32 (x, (y & 255) |
> (z << 8)); or similar)?

It looks to me that GCC's double-underscored version is wrong. We are
looking for compatibility with official bmiintrin.h header. Kirill,
can you please comment on this issue?

> 2013-06-17  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/57623
>         * config/i386/i386.md (bmi_bextr_<mode>): Swap predicates and
>         constraints of operand 1 and 2.
>
>         * gcc.target/i386/bmi-bextr-3.c: New test.

The fix itself looks good to me as far as insn is concerned, but
please wait until the issue with builtins is resolved.

Thanks,
Uros.
Jakub Jelinek - June 18, 2013, 9:51 a.m.
On Tue, Jun 18, 2013 at 11:47:29AM +0200, Uros Bizjak wrote:
> On Mon, Jun 17, 2013 at 6:11 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> 
> > This instruction has the predicates/constraints wrong, the r/m argument is
> > the middle-one, the value from which it should be extracted, rather than
> > the packed start/length argument.  This got broken with PR50766, where the
> > patch hasn't touched just BMI2, but for unknown reasons also this BMI
> > instruction which was handled right in GCC 4.6.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8/4.7?
> >
> > BTW, the AVX2 docs document _bextr_u{32,64} 3 argument intrinsics, rather
> > than the __bextr_u{32,64} 2 argument intrinsics we have in GCC right now.
> > Something to fix up (as the names are different, perhaps we can both the old
> > ones and the new ones implemented say as return __bextr_u32 (x, (y & 255) |
> > (z << 8)); or similar)?
> 
> It looks to me that GCC's double-underscored version is wrong. We are
> looking for compatibility with official bmiintrin.h header. Kirill,
> can you please comment on this issue?

Well, bmiintrin.h has been added first as an AMD ISA extension, and I think for
AMD extensions the GCC headers are the official place (the AMD docs don't
mention the intrinsics at all), and only later on also added as Intel ISA
extension.  So I'd say it is fine to keep the __bextr_* variants (as written
in the PR, sometimes the two argument ones can be useful if you e.g.
precompute the start/length pairs ahead of time) and just add the one
underscored ones to match ICC/AVX2 documentation.

	Jakub
Uros Bizjak - June 18, 2013, 10:12 a.m.
On Tue, Jun 18, 2013 at 11:51 AM, Jakub Jelinek <jakub@redhat.com> wrote:

>> > This instruction has the predicates/constraints wrong, the r/m argument is
>> > the middle-one, the value from which it should be extracted, rather than
>> > the packed start/length argument.  This got broken with PR50766, where the
>> > patch hasn't touched just BMI2, but for unknown reasons also this BMI
>> > instruction which was handled right in GCC 4.6.
>> >
>> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk/4.8/4.7?
>> >
>> > BTW, the AVX2 docs document _bextr_u{32,64} 3 argument intrinsics, rather
>> > than the __bextr_u{32,64} 2 argument intrinsics we have in GCC right now.
>> > Something to fix up (as the names are different, perhaps we can both the old
>> > ones and the new ones implemented say as return __bextr_u32 (x, (y & 255) |
>> > (z << 8)); or similar)?
>>
>> It looks to me that GCC's double-underscored version is wrong. We are
>> looking for compatibility with official bmiintrin.h header. Kirill,
>> can you please comment on this issue?
>
> Well, bmiintrin.h has been added first as an AMD ISA extension, and I think for
> AMD extensions the GCC headers are the official place (the AMD docs don't
> mention the intrinsics at all), and only later on also added as Intel ISA
> extension.  So I'd say it is fine to keep the __bextr_* variants (as written
> in the PR, sometimes the two argument ones can be useful if you e.g.
> precompute the start/length pairs ahead of time) and just add the one
> underscored ones to match ICC/AVX2 documentation.

I agree with this proposal, but probably we need to review the whole
bmiintrin.h for intel additions.

Uros.

Patch

--- gcc/config/i386/i386.md.jj	2013-06-09 20:29:02.000000000 +0200
+++ gcc/config/i386/i386.md	2013-06-15 18:24:48.942362202 +0200
@@ -11679,8 +11679,8 @@  (define_insn "*bmi_andn_<mode>"
 
 (define_insn "bmi_bextr_<mode>"
   [(set (match_operand:SWI48 0 "register_operand" "=r,r")
-        (unspec:SWI48 [(match_operand:SWI48 1 "register_operand" "r,r")
-                       (match_operand:SWI48 2 "nonimmediate_operand" "r,m")]
+        (unspec:SWI48 [(match_operand:SWI48 1 "nonimmediate_operand" "r,m")
+                       (match_operand:SWI48 2 "register_operand" "r,r")]
                        UNSPEC_BEXTR))
    (clobber (reg:CC FLAGS_REG))]
   "TARGET_BMI"
--- gcc/testsuite/gcc.target/i386/bmi-bextr-3.c.jj	2013-06-17 09:48:47.789600664 +0200
+++ gcc/testsuite/gcc.target/i386/bmi-bextr-3.c	2013-06-17 09:48:37.000000000 +0200
@@ -0,0 +1,31 @@ 
+/* PR target/57623 */
+/* { dg-do assemble { target bmi } } */
+/* { dg-options "-O2 -mbmi" } */
+
+#include <x86intrin.h>
+
+unsigned int
+f1 (unsigned int x, unsigned int *y)
+{
+  return __bextr_u32 (x, *y);
+}
+
+unsigned int
+f2 (unsigned int *x, unsigned int y)
+{
+  return __bextr_u32 (*x, y);
+}
+
+#ifdef  __x86_64__
+unsigned long long
+f3 (unsigned long long x, unsigned long long *y)
+{
+  return __bextr_u64 (x, *y);
+}
+
+unsigned long long
+f4 (unsigned long long *x, unsigned long long y)
+{
+  return __bextr_u64 (*x, y);
+}
+#endif