| Message ID | 20130617161152.GZ2336@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
--- 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
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