i386: Correct target attribute for crc32 intrinsics

Message ID 20220415042942.77387-1-hongyu.wang@intel.com
State New
Headers
Series i386: Correct target attribute for crc32 intrinsics |

Commit Message

Hongyu Wang April 15, 2022, 4:29 a.m. UTC
  Hi,

Complile _mm_crc32_u8/16/32/64 intrinsics with -mcrc32
would meet target specific option mismatch. Correct target pragma
to fix.

Bootstrapped/regtest on x86_64-pc-linux-gnu{-m32,}.

Ok for master and backport to GCC 11?

gcc/ChangeLog:

	* config/i386/smmintrin.h: Correct target pragma from sse4.1
	and sse4.2 to crc32 for crc32 intrinsics.

gcc/testsuite/ChangeLog:

	* gcc.target/i386/crc32-6.c: Adjust to call builtin.
	* gcc.target/i386/crc32-7.c: New test.
---
 gcc/config/i386/smmintrin.h             | 25 +++++-------------
 gcc/testsuite/gcc.target/i386/crc32-6.c |  2 +-
 gcc/testsuite/gcc.target/i386/crc32-7.c | 34 +++++++++++++++++++++++++
 3 files changed, 42 insertions(+), 19 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/i386/crc32-7.c
  

Comments

Uros Bizjak April 15, 2022, 7:54 a.m. UTC | #1
On Fri, Apr 15, 2022 at 6:30 AM Hongyu Wang <hongyu.wang@intel.com> wrote:
>
> Hi,
>
> Complile _mm_crc32_u8/16/32/64 intrinsics with -mcrc32
> would meet target specific option mismatch. Correct target pragma
> to fix.
>
> Bootstrapped/regtest on x86_64-pc-linux-gnu{-m32,}.
>
> Ok for master and backport to GCC 11?
>
> gcc/ChangeLog:
>
>         * config/i386/smmintrin.h: Correct target pragma from sse4.1
>         and sse4.2 to crc32 for crc32 intrinsics.
>
> gcc/testsuite/ChangeLog:
>
>         * gcc.target/i386/crc32-6.c: Adjust to call builtin.
>         * gcc.target/i386/crc32-7.c: New test.
> ---
>  gcc/config/i386/smmintrin.h             | 25 +++++-------------
>  gcc/testsuite/gcc.target/i386/crc32-6.c |  2 +-
>  gcc/testsuite/gcc.target/i386/crc32-7.c | 34 +++++++++++++++++++++++++
>  3 files changed, 42 insertions(+), 19 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/i386/crc32-7.c
>
> diff --git a/gcc/config/i386/smmintrin.h b/gcc/config/i386/smmintrin.h
> index b42b212300f..eb6a451c10a 100644
> --- a/gcc/config/i386/smmintrin.h
> +++ b/gcc/config/i386/smmintrin.h
> @@ -810,17 +810,11 @@ _mm_cmpgt_epi64 (__m128i __X, __m128i __Y)
>
>  #include <popcntintrin.h>
>
> -#ifndef __SSE4_1__
> +#ifndef __CRC32__
>  #pragma GCC push_options
> -#pragma GCC target("sse4.1")
> -#define __DISABLE_SSE4_1__
> -#endif /* __SSE4_1__ */
> -
> -#ifndef __SSE4_2__
> -#pragma GCC push_options
> -#pragma GCC target("sse4.2")
> -#define __DISABLE_SSE4_2__
> -#endif /* __SSE4_1__ */
> +#pragma GCC target("crc32")
> +#define __DISABLE_CRC32__
> +#endif /* __CRC32__ */
>
>  /* Accumulate CRC32 (polynomial 0x11EDC6F41) value.  */
>  extern __inline unsigned int __attribute__((__gnu_inline__, __always_inline__, __artificial__))
> @@ -849,14 +843,9 @@ _mm_crc32_u64 (unsigned long long __C, unsigned long long __V)
>  }
>  #endif
>
> -#ifdef __DISABLE_SSE4_2__
> -#undef __DISABLE_SSE4_2__
> +#ifdef __DISABLE_CRC32__
> +#undef __DISABLE_CRC32__
>  #pragma GCC pop_options
> -#endif /* __DISABLE_SSE4_2__ */
> -
> -#ifdef __DISABLE_SSE4_1__
> -#undef __DISABLE_SSE4_1__
> -#pragma GCC pop_options
> -#endif /* __DISABLE_SSE4_1__ */
> +#endif /* __DISABLE_CRC32__ */
>
>  #endif /* _SMMINTRIN_H_INCLUDED */
> diff --git a/gcc/testsuite/gcc.target/i386/crc32-6.c b/gcc/testsuite/gcc.target/i386/crc32-6.c
> index 464e3444069..1f306534bb8 100644
> --- a/gcc/testsuite/gcc.target/i386/crc32-6.c
> +++ b/gcc/testsuite/gcc.target/i386/crc32-6.c
> @@ -7,7 +7,7 @@
>  unsigned int
>  test_mm_crc32_u8 (unsigned int CRC, unsigned char V)
>  {
> -  return _mm_crc32_u8 (CRC, V);
> +  return __builtin_ia32_crc32qi (CRC, V);

This test should not be changed, it correctly reports ISA mismatch. It
even passes -mno-crc32.

Uros.

>  }
>
>  /* { dg-error "needs isa option -mcrc32" "" { target *-*-* } 0  } */
> diff --git a/gcc/testsuite/gcc.target/i386/crc32-7.c b/gcc/testsuite/gcc.target/i386/crc32-7.c
> new file mode 100644
> index 00000000000..2e310e38b82
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/crc32-7.c
> @@ -0,0 +1,34 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mcrc32" } */
> +/* { dg-final { scan-assembler "crc32b\[^\\n\]*eax" } } */
> +/* { dg-final { scan-assembler "crc32w\[^\\n\]*eax" } } */
> +/* { dg-final { scan-assembler "crc32l\[^\\n\]*eax" } } */
> +/* { dg-final { scan-assembler "crc32q\[^\\n\]*rax" { target { ! ia32 } } } } */
> +
> +#include <immintrin.h>
> +
> +unsigned int
> +test_mm_crc32_u8 (unsigned int CRC, unsigned char V)
> +{
> +  return _mm_crc32_u8 (CRC, V);
> +}
> +
> +unsigned int
> +test_mm_crc32_u16 (unsigned int CRC, unsigned short V)
> +{
> +  return _mm_crc32_u16 (CRC, V);
> +}
> +
> +unsigned int
> +test_mm_crc32_u32 (unsigned int CRC, unsigned int V)
> +{
> +  return _mm_crc32_u32 (CRC, V);
> +}
> +
> +#ifdef __x86_64__
> +unsigned long long
> +test_mm_crc32_u64 (unsigned long long CRC, unsigned long long V)
> +{
> +  return _mm_crc32_u64 (CRC, V);
> +}
> +#endif
> --
> 2.18.1
>
  
Hongyu Wang April 15, 2022, 8:33 a.m. UTC | #2
> This test should not be changed, it correctly reports ISA mismatch. It
> even passes -mno-crc32.

The error message changes from "needs isa option -mcrc32" to "target
specific option mismatch" with the #pragma change.
I see many of our intrinsic would throw such error, it has been a long
term issue for intrinsic diagnostic.

So for this test either I change the dg-error message or the call to
builtin, otherwise it would fail.

Uros Bizjak via Gcc-patches <gcc-patches@gcc.gnu.org> 于2022年4月15日周五 15:54写道:
>
> On Fri, Apr 15, 2022 at 6:30 AM Hongyu Wang <hongyu.wang@intel.com> wrote:
> >
> > Hi,
> >
> > Complile _mm_crc32_u8/16/32/64 intrinsics with -mcrc32
> > would meet target specific option mismatch. Correct target pragma
> > to fix.
> >
> > Bootstrapped/regtest on x86_64-pc-linux-gnu{-m32,}.
> >
> > Ok for master and backport to GCC 11?
> >
> > gcc/ChangeLog:
> >
> >         * config/i386/smmintrin.h: Correct target pragma from sse4.1
> >         and sse4.2 to crc32 for crc32 intrinsics.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.target/i386/crc32-6.c: Adjust to call builtin.
> >         * gcc.target/i386/crc32-7.c: New test.
> > ---
> >  gcc/config/i386/smmintrin.h             | 25 +++++-------------
> >  gcc/testsuite/gcc.target/i386/crc32-6.c |  2 +-
> >  gcc/testsuite/gcc.target/i386/crc32-7.c | 34 +++++++++++++++++++++++++
> >  3 files changed, 42 insertions(+), 19 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.target/i386/crc32-7.c
> >
> > diff --git a/gcc/config/i386/smmintrin.h b/gcc/config/i386/smmintrin.h
> > index b42b212300f..eb6a451c10a 100644
> > --- a/gcc/config/i386/smmintrin.h
> > +++ b/gcc/config/i386/smmintrin.h
> > @@ -810,17 +810,11 @@ _mm_cmpgt_epi64 (__m128i __X, __m128i __Y)
> >
> >  #include <popcntintrin.h>
> >
> > -#ifndef __SSE4_1__
> > +#ifndef __CRC32__
> >  #pragma GCC push_options
> > -#pragma GCC target("sse4.1")
> > -#define __DISABLE_SSE4_1__
> > -#endif /* __SSE4_1__ */
> > -
> > -#ifndef __SSE4_2__
> > -#pragma GCC push_options
> > -#pragma GCC target("sse4.2")
> > -#define __DISABLE_SSE4_2__
> > -#endif /* __SSE4_1__ */
> > +#pragma GCC target("crc32")
> > +#define __DISABLE_CRC32__
> > +#endif /* __CRC32__ */
> >
> >  /* Accumulate CRC32 (polynomial 0x11EDC6F41) value.  */
> >  extern __inline unsigned int __attribute__((__gnu_inline__, __always_inline__, __artificial__))
> > @@ -849,14 +843,9 @@ _mm_crc32_u64 (unsigned long long __C, unsigned long long __V)
> >  }
> >  #endif
> >
> > -#ifdef __DISABLE_SSE4_2__
> > -#undef __DISABLE_SSE4_2__
> > +#ifdef __DISABLE_CRC32__
> > +#undef __DISABLE_CRC32__
> >  #pragma GCC pop_options
> > -#endif /* __DISABLE_SSE4_2__ */
> > -
> > -#ifdef __DISABLE_SSE4_1__
> > -#undef __DISABLE_SSE4_1__
> > -#pragma GCC pop_options
> > -#endif /* __DISABLE_SSE4_1__ */
> > +#endif /* __DISABLE_CRC32__ */
> >
> >  #endif /* _SMMINTRIN_H_INCLUDED */
> > diff --git a/gcc/testsuite/gcc.target/i386/crc32-6.c b/gcc/testsuite/gcc.target/i386/crc32-6.c
> > index 464e3444069..1f306534bb8 100644
> > --- a/gcc/testsuite/gcc.target/i386/crc32-6.c
> > +++ b/gcc/testsuite/gcc.target/i386/crc32-6.c
> > @@ -7,7 +7,7 @@
> >  unsigned int
> >  test_mm_crc32_u8 (unsigned int CRC, unsigned char V)
> >  {
> > -  return _mm_crc32_u8 (CRC, V);
> > +  return __builtin_ia32_crc32qi (CRC, V);
>
> This test should not be changed, it correctly reports ISA mismatch. It
> even passes -mno-crc32.
>
> Uros.
>
> >  }
> >
> >  /* { dg-error "needs isa option -mcrc32" "" { target *-*-* } 0  } */
> > diff --git a/gcc/testsuite/gcc.target/i386/crc32-7.c b/gcc/testsuite/gcc.target/i386/crc32-7.c
> > new file mode 100644
> > index 00000000000..2e310e38b82
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.target/i386/crc32-7.c
> > @@ -0,0 +1,34 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -mcrc32" } */
> > +/* { dg-final { scan-assembler "crc32b\[^\\n\]*eax" } } */
> > +/* { dg-final { scan-assembler "crc32w\[^\\n\]*eax" } } */
> > +/* { dg-final { scan-assembler "crc32l\[^\\n\]*eax" } } */
> > +/* { dg-final { scan-assembler "crc32q\[^\\n\]*rax" { target { ! ia32 } } } } */
> > +
> > +#include <immintrin.h>
> > +
> > +unsigned int
> > +test_mm_crc32_u8 (unsigned int CRC, unsigned char V)
> > +{
> > +  return _mm_crc32_u8 (CRC, V);
> > +}
> > +
> > +unsigned int
> > +test_mm_crc32_u16 (unsigned int CRC, unsigned short V)
> > +{
> > +  return _mm_crc32_u16 (CRC, V);
> > +}
> > +
> > +unsigned int
> > +test_mm_crc32_u32 (unsigned int CRC, unsigned int V)
> > +{
> > +  return _mm_crc32_u32 (CRC, V);
> > +}
> > +
> > +#ifdef __x86_64__
> > +unsigned long long
> > +test_mm_crc32_u64 (unsigned long long CRC, unsigned long long V)
> > +{
> > +  return _mm_crc32_u64 (CRC, V);
> > +}
> > +#endif
> > --
> > 2.18.1
> >
  
Uros Bizjak April 15, 2022, 11:44 a.m. UTC | #3
On Fri, Apr 15, 2022 at 10:34 AM Hongyu Wang <wwwhhhyyy333@gmail.com> wrote:
>
> > This test should not be changed, it correctly reports ISA mismatch. It
> > even passes -mno-crc32.
>
> The error message changes from "needs isa option -mcrc32" to "target
> specific option mismatch" with the #pragma change.
> I see many of our intrinsic would throw such error, it has been a long
> term issue for intrinsic diagnostic.
>
> So for this test either I change the dg-error message or the call to
> builtin, otherwise it would fail.

Please update the dg-error message.

OK with the above change.

Thanks,
Uros.

>
> Uros Bizjak via Gcc-patches <gcc-patches@gcc.gnu.org> 于2022年4月15日周五 15:54写道:
> >
> > On Fri, Apr 15, 2022 at 6:30 AM Hongyu Wang <hongyu.wang@intel.com> wrote:
> > >
> > > Hi,
> > >
> > > Complile _mm_crc32_u8/16/32/64 intrinsics with -mcrc32
> > > would meet target specific option mismatch. Correct target pragma
> > > to fix.
> > >
> > > Bootstrapped/regtest on x86_64-pc-linux-gnu{-m32,}.
> > >
> > > Ok for master and backport to GCC 11?
> > >
> > > gcc/ChangeLog:
> > >
> > >         * config/i386/smmintrin.h: Correct target pragma from sse4.1
> > >         and sse4.2 to crc32 for crc32 intrinsics.
> > >
> > > gcc/testsuite/ChangeLog:
> > >
> > >         * gcc.target/i386/crc32-6.c: Adjust to call builtin.
> > >         * gcc.target/i386/crc32-7.c: New test.
> > > ---
> > >  gcc/config/i386/smmintrin.h             | 25 +++++-------------
> > >  gcc/testsuite/gcc.target/i386/crc32-6.c |  2 +-
> > >  gcc/testsuite/gcc.target/i386/crc32-7.c | 34 +++++++++++++++++++++++++
> > >  3 files changed, 42 insertions(+), 19 deletions(-)
> > >  create mode 100644 gcc/testsuite/gcc.target/i386/crc32-7.c
> > >
> > > diff --git a/gcc/config/i386/smmintrin.h b/gcc/config/i386/smmintrin.h
> > > index b42b212300f..eb6a451c10a 100644
> > > --- a/gcc/config/i386/smmintrin.h
> > > +++ b/gcc/config/i386/smmintrin.h
> > > @@ -810,17 +810,11 @@ _mm_cmpgt_epi64 (__m128i __X, __m128i __Y)
> > >
> > >  #include <popcntintrin.h>
> > >
> > > -#ifndef __SSE4_1__
> > > +#ifndef __CRC32__
> > >  #pragma GCC push_options
> > > -#pragma GCC target("sse4.1")
> > > -#define __DISABLE_SSE4_1__
> > > -#endif /* __SSE4_1__ */
> > > -
> > > -#ifndef __SSE4_2__
> > > -#pragma GCC push_options
> > > -#pragma GCC target("sse4.2")
> > > -#define __DISABLE_SSE4_2__
> > > -#endif /* __SSE4_1__ */
> > > +#pragma GCC target("crc32")
> > > +#define __DISABLE_CRC32__
> > > +#endif /* __CRC32__ */
> > >
> > >  /* Accumulate CRC32 (polynomial 0x11EDC6F41) value.  */
> > >  extern __inline unsigned int __attribute__((__gnu_inline__, __always_inline__, __artificial__))
> > > @@ -849,14 +843,9 @@ _mm_crc32_u64 (unsigned long long __C, unsigned long long __V)
> > >  }
> > >  #endif
> > >
> > > -#ifdef __DISABLE_SSE4_2__
> > > -#undef __DISABLE_SSE4_2__
> > > +#ifdef __DISABLE_CRC32__
> > > +#undef __DISABLE_CRC32__
> > >  #pragma GCC pop_options
> > > -#endif /* __DISABLE_SSE4_2__ */
> > > -
> > > -#ifdef __DISABLE_SSE4_1__
> > > -#undef __DISABLE_SSE4_1__
> > > -#pragma GCC pop_options
> > > -#endif /* __DISABLE_SSE4_1__ */
> > > +#endif /* __DISABLE_CRC32__ */
> > >
> > >  #endif /* _SMMINTRIN_H_INCLUDED */
> > > diff --git a/gcc/testsuite/gcc.target/i386/crc32-6.c b/gcc/testsuite/gcc.target/i386/crc32-6.c
> > > index 464e3444069..1f306534bb8 100644
> > > --- a/gcc/testsuite/gcc.target/i386/crc32-6.c
> > > +++ b/gcc/testsuite/gcc.target/i386/crc32-6.c
> > > @@ -7,7 +7,7 @@
> > >  unsigned int
> > >  test_mm_crc32_u8 (unsigned int CRC, unsigned char V)
> > >  {
> > > -  return _mm_crc32_u8 (CRC, V);
> > > +  return __builtin_ia32_crc32qi (CRC, V);
> >
> > This test should not be changed, it correctly reports ISA mismatch. It
> > even passes -mno-crc32.
> >
> > Uros.
> >
> > >  }
> > >
> > >  /* { dg-error "needs isa option -mcrc32" "" { target *-*-* } 0  } */
> > > diff --git a/gcc/testsuite/gcc.target/i386/crc32-7.c b/gcc/testsuite/gcc.target/i386/crc32-7.c
> > > new file mode 100644
> > > index 00000000000..2e310e38b82
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.target/i386/crc32-7.c
> > > @@ -0,0 +1,34 @@
> > > +/* { dg-do compile } */
> > > +/* { dg-options "-O2 -mcrc32" } */
> > > +/* { dg-final { scan-assembler "crc32b\[^\\n\]*eax" } } */
> > > +/* { dg-final { scan-assembler "crc32w\[^\\n\]*eax" } } */
> > > +/* { dg-final { scan-assembler "crc32l\[^\\n\]*eax" } } */
> > > +/* { dg-final { scan-assembler "crc32q\[^\\n\]*rax" { target { ! ia32 } } } } */
> > > +
> > > +#include <immintrin.h>
> > > +
> > > +unsigned int
> > > +test_mm_crc32_u8 (unsigned int CRC, unsigned char V)
> > > +{
> > > +  return _mm_crc32_u8 (CRC, V);
> > > +}
> > > +
> > > +unsigned int
> > > +test_mm_crc32_u16 (unsigned int CRC, unsigned short V)
> > > +{
> > > +  return _mm_crc32_u16 (CRC, V);
> > > +}
> > > +
> > > +unsigned int
> > > +test_mm_crc32_u32 (unsigned int CRC, unsigned int V)
> > > +{
> > > +  return _mm_crc32_u32 (CRC, V);
> > > +}
> > > +
> > > +#ifdef __x86_64__
> > > +unsigned long long
> > > +test_mm_crc32_u64 (unsigned long long CRC, unsigned long long V)
> > > +{
> > > +  return _mm_crc32_u64 (CRC, V);
> > > +}
> > > +#endif
> > > --
> > > 2.18.1
> > >
  

Patch

diff --git a/gcc/config/i386/smmintrin.h b/gcc/config/i386/smmintrin.h
index b42b212300f..eb6a451c10a 100644
--- a/gcc/config/i386/smmintrin.h
+++ b/gcc/config/i386/smmintrin.h
@@ -810,17 +810,11 @@  _mm_cmpgt_epi64 (__m128i __X, __m128i __Y)
 
 #include <popcntintrin.h>
 
-#ifndef __SSE4_1__
+#ifndef __CRC32__
 #pragma GCC push_options
-#pragma GCC target("sse4.1")
-#define __DISABLE_SSE4_1__
-#endif /* __SSE4_1__ */
-
-#ifndef __SSE4_2__
-#pragma GCC push_options
-#pragma GCC target("sse4.2")
-#define __DISABLE_SSE4_2__
-#endif /* __SSE4_1__ */
+#pragma GCC target("crc32")
+#define __DISABLE_CRC32__
+#endif /* __CRC32__ */
 
 /* Accumulate CRC32 (polynomial 0x11EDC6F41) value.  */
 extern __inline unsigned int __attribute__((__gnu_inline__, __always_inline__, __artificial__))
@@ -849,14 +843,9 @@  _mm_crc32_u64 (unsigned long long __C, unsigned long long __V)
 }
 #endif
 
-#ifdef __DISABLE_SSE4_2__
-#undef __DISABLE_SSE4_2__
+#ifdef __DISABLE_CRC32__
+#undef __DISABLE_CRC32__
 #pragma GCC pop_options
-#endif /* __DISABLE_SSE4_2__ */
-
-#ifdef __DISABLE_SSE4_1__
-#undef __DISABLE_SSE4_1__
-#pragma GCC pop_options
-#endif /* __DISABLE_SSE4_1__ */
+#endif /* __DISABLE_CRC32__ */
 
 #endif /* _SMMINTRIN_H_INCLUDED */
diff --git a/gcc/testsuite/gcc.target/i386/crc32-6.c b/gcc/testsuite/gcc.target/i386/crc32-6.c
index 464e3444069..1f306534bb8 100644
--- a/gcc/testsuite/gcc.target/i386/crc32-6.c
+++ b/gcc/testsuite/gcc.target/i386/crc32-6.c
@@ -7,7 +7,7 @@ 
 unsigned int
 test_mm_crc32_u8 (unsigned int CRC, unsigned char V)
 {
-  return _mm_crc32_u8 (CRC, V);
+  return __builtin_ia32_crc32qi (CRC, V);
 }
 
 /* { dg-error "needs isa option -mcrc32" "" { target *-*-* } 0  } */
diff --git a/gcc/testsuite/gcc.target/i386/crc32-7.c b/gcc/testsuite/gcc.target/i386/crc32-7.c
new file mode 100644
index 00000000000..2e310e38b82
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/crc32-7.c
@@ -0,0 +1,34 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2 -mcrc32" } */
+/* { dg-final { scan-assembler "crc32b\[^\\n\]*eax" } } */
+/* { dg-final { scan-assembler "crc32w\[^\\n\]*eax" } } */
+/* { dg-final { scan-assembler "crc32l\[^\\n\]*eax" } } */
+/* { dg-final { scan-assembler "crc32q\[^\\n\]*rax" { target { ! ia32 } } } } */
+
+#include <immintrin.h>
+
+unsigned int
+test_mm_crc32_u8 (unsigned int CRC, unsigned char V)
+{
+  return _mm_crc32_u8 (CRC, V);
+}
+
+unsigned int
+test_mm_crc32_u16 (unsigned int CRC, unsigned short V)
+{
+  return _mm_crc32_u16 (CRC, V);
+}
+
+unsigned int
+test_mm_crc32_u32 (unsigned int CRC, unsigned int V)
+{
+  return _mm_crc32_u32 (CRC, V);
+}
+
+#ifdef __x86_64__
+unsigned long long
+test_mm_crc32_u64 (unsigned long long CRC, unsigned long long V)
+{
+  return _mm_crc32_u64 (CRC, V);
+}
+#endif