diff mbox series

[v1] lib/hash: support non sse42 cpu architecture

Message ID 20210112072446.880122-1-kumar.amber@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers
Series [v1] lib/hash: support non sse42 cpu architecture |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS

Commit Message

Amber, Kumar Jan. 12, 2021, 7:24 a.m. UTC 
  add _SSE42_ flag to enable compilation of
sse42 specific instructions only on supported
architecture

Signed-off-by: kumar amber <kumar.amber@intel.com>
---
 lib/librte_hash/rte_hash_crc.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Thomas Monjalon March 24, 2021, 9:20 p.m. UTC | #1 
There was no review of this patch in last 2 months.
+Cc Yipeng and Sameh


12/01/2021 08:24, kumar amber:
> add _SSE42_ flag to enable compilation of
> sse42 specific instructions only on supported
> architecture
> 
> Signed-off-by: kumar amber <kumar.amber@intel.com>
> ---
>  lib/librte_hash/rte_hash_crc.h | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
> index 3e131aa6bb..e9f063780c 100644
> --- a/lib/librte_hash/rte_hash_crc.h
> +++ b/lib/librte_hash/rte_hash_crc.h
> @@ -358,7 +358,7 @@ crc32c_2words(uint64_t data, uint32_t init_val)
>  	return crc;
>  }
>  
> -#if defined(RTE_ARCH_X86)
> +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
>  static inline uint32_t
>  crc32c_sse42_u8(uint8_t data, uint32_t init_val)
>  {
> @@ -404,7 +404,7 @@ crc32c_sse42_u64_mimic(uint64_t data, uint64_t init_val)
>  }
>  #endif
>  
> -#ifdef RTE_ARCH_X86_64
> +#if defined(RTE_ARCH_X86_64) && defined(__SSE42__)
>  static inline uint32_t
>  crc32c_sse42_u64(uint64_t data, uint64_t init_val)
>  {
> @@ -442,7 +442,7 @@ static uint8_t crc32_alg = CRC32_SW;
>  static inline void
>  rte_hash_crc_set_alg(uint8_t alg)
>  {
> -#if defined(RTE_ARCH_X86)
> +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
>  	if (alg == CRC32_SSE42_x64 &&
>  			!rte_cpu_get_flag_enabled(RTE_CPUFLAG_EM64T))
>  		alg = CRC32_SSE42;
> @@ -471,7 +471,7 @@ RTE_INIT(rte_hash_crc_init_alg)
>  static inline uint32_t
>  rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
>  {
> -#if defined RTE_ARCH_X86
> +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
>  	if (likely(crc32_alg & CRC32_SSE42))
>  		return crc32c_sse42_u8(data, init_val);
>  #endif
> @@ -494,7 +494,7 @@ rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
>  static inline uint32_t
>  rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
>  {
> -#if defined RTE_ARCH_X86
> +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
>  	if (likely(crc32_alg & CRC32_SSE42))
>  		return crc32c_sse42_u16(data, init_val);
>  #endif
> @@ -517,7 +517,7 @@ rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
>  static inline uint32_t
>  rte_hash_crc_4byte(uint32_t data, uint32_t init_val)
>  {
> -#if defined RTE_ARCH_X86
> +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
>  	if (likely(crc32_alg & CRC32_SSE42))
>  		return crc32c_sse42_u32(data, init_val);
>  #endif
> @@ -540,12 +540,12 @@ rte_hash_crc_4byte(uint32_t data, uint32_t init_val)
>  static inline uint32_t
>  rte_hash_crc_8byte(uint64_t data, uint32_t init_val)
>  {
> -#ifdef RTE_ARCH_X86_64
> +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
>  	if (likely(crc32_alg == CRC32_SSE42_x64))
>  		return crc32c_sse42_u64(data, init_val);
>  #endif
>  
> -#if defined RTE_ARCH_X86
> +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
>  	if (likely(crc32_alg & CRC32_SSE42))
>  		return crc32c_sse42_u64_mimic(data, init_val);
>  #endif
>
Wang, Yipeng1 March 24, 2021, 10:59 p.m. UTC | #2 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of kumar amber
> Sent: Monday, January 11, 2021 11:25 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>
> Subject: [dpdk-dev] [PATCH v1] lib/hash: support non sse42 cpu architecture
> 
> add _SSE42_ flag to enable compilation of
> sse42 specific instructions only on supported architecture
> 
> Signed-off-by: kumar amber <kumar.amber@intel.com>
> ---
>  lib/librte_hash/rte_hash_crc.h | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
> index 3e131aa6bb..e9f063780c 100644
> --- a/lib/librte_hash/rte_hash_crc.h
> +++ b/lib/librte_hash/rte_hash_crc.h
> @@ -358,7 +358,7 @@ crc32c_2words(uint64_t data, uint32_t init_val)
>  	return crc;
>  }
> 
> -#if defined(RTE_ARCH_X86)
> +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
>  static inline uint32_t
>  crc32c_sse42_u8(uint8_t data, uint32_t init_val)  { @@ -404,7 +404,7 @@
> crc32c_sse42_u64_mimic(uint64_t data, uint64_t init_val)  }  #endif

...

> -#if defined RTE_ARCH_X86
> +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
>  	if (likely(crc32_alg & CRC32_SSE42))
>  		return crc32c_sse42_u64_mimic(data, init_val);  #endif
> --
> 2.25.1

[Wang, Yipeng] 
Hi, Kumar, thanks for the patch.
I think the minimum required machine for x86 is sse4.2 compatible already. So I wonder if we really need this.

Also, I think the right way to check machine flag in DPDK should be:
#If defined (RTE_MACHINE_CPUFLAG_SSE4_2)
Instead of using compiler dependent macro.
Thomas Monjalon March 25, 2021, 8:06 a.m. UTC | #3 
24/03/2021 23:59, Wang, Yipeng1:
> From: kumar amber
> > 
> > add _SSE42_ flag to enable compilation of
> > sse42 specific instructions only on supported architecture
> > 
> > Signed-off-by: kumar amber <kumar.amber@intel.com>
> > ---
> >  lib/librte_hash/rte_hash_crc.h | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
> > index 3e131aa6bb..e9f063780c 100644
> > --- a/lib/librte_hash/rte_hash_crc.h
> > +++ b/lib/librte_hash/rte_hash_crc.h
> > @@ -358,7 +358,7 @@ crc32c_2words(uint64_t data, uint32_t init_val)
> >  	return crc;
> >  }
> > 
> > -#if defined(RTE_ARCH_X86)
> > +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
> >  static inline uint32_t
> >  crc32c_sse42_u8(uint8_t data, uint32_t init_val)  { @@ -404,7 +404,7 @@
> > crc32c_sse42_u64_mimic(uint64_t data, uint64_t init_val)  }  #endif
> 
> ...
> 
> > -#if defined RTE_ARCH_X86
> > +#if defined(RTE_ARCH_X86) && defined(__SSE42__)
> >  	if (likely(crc32_alg & CRC32_SSE42))
> >  		return crc32c_sse42_u64_mimic(data, init_val);  #endif
> > --
> > 2.25.1
> 
> [Wang, Yipeng] 
> Hi, Kumar, thanks for the patch.
> I think the minimum required machine for x86 is sse4.2 compatible already. So I wonder if we really need this.

Yes, that's why I don't understand this patch.

> Also, I think the right way to check machine flag in DPDK should be:
> #If defined (RTE_MACHINE_CPUFLAG_SSE4_2)

These macros have been removed in DPDK 20.11.

> Instead of using compiler dependent macro.

Compiler macros are well standardized, it is OK.
Thomas Monjalon April 8, 2021, 10:41 p.m. UTC | #4 
25/03/2021 09:06, Thomas Monjalon:
> 24/03/2021 23:59, Wang, Yipeng1:
> > From: kumar amber
> > > 
> > > add _SSE42_ flag to enable compilation of
> > > sse42 specific instructions only on supported architecture
> > > 
> > > Signed-off-by: kumar amber <kumar.amber@intel.com>
[...]
> > [Wang, Yipeng] 
> > Hi, Kumar, thanks for the patch.
> > I think the minimum required machine for x86 is sse4.2 compatible already. So I wonder if we really need this.
> 
> Yes, that's why I don't understand this patch.

Any comment? Should we reject the patch?
Stephen Hemminger April 8, 2021, 11:01 p.m. UTC | #5 
On Fri, 09 Apr 2021 00:41:34 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 25/03/2021 09:06, Thomas Monjalon:
> > 24/03/2021 23:59, Wang, Yipeng1:  
> > > From: kumar amber  
> > > > 
> > > > add _SSE42_ flag to enable compilation of
> > > > sse42 specific instructions only on supported architecture
> > > > 
> > > > Signed-off-by: kumar amber <kumar.amber@intel.com>  
> [...]
> > > [Wang, Yipeng] 
> > > Hi, Kumar, thanks for the patch.
> > > I think the minimum required machine for x86 is sse4.2 compatible already. So I wonder if we really need this.  
> > 
> > Yes, that's why I don't understand this patch.  
> 
> Any comment? Should we reject the patch?
> 
> 
> 

The only use case would be running DPDK in a VM for a hypervisor that
disables those instructions in guest. Some old crufty hypervisors don't
handle it??
Amber, Kumar April 9, 2021, 3:17 a.m. UTC | #6 
Hi Thomas ,

Pls reject it 

-----Original Message-----
From: Thomas Monjalon <thomas@monjalon.net> 
Sent: Friday, April 9, 2021 4:12 AM
To: Amber, Kumar <kumar.amber@intel.com>
Cc: Wang, Yipeng1 <yipeng1.wang@intel.com>; dev@dpdk.org; Richardson, Bruce <bruce.richardson@intel.com>; Gobriel, Sameh <sameh.gobriel@intel.com>
Subject: Re: [dpdk-dev] [PATCH v1] lib/hash: support non sse42 cpu architecture

25/03/2021 09:06, Thomas Monjalon:
> 24/03/2021 23:59, Wang, Yipeng1:
> > From: kumar amber
> > > 
> > > add _SSE42_ flag to enable compilation of
> > > sse42 specific instructions only on supported architecture
> > > 
> > > Signed-off-by: kumar amber <kumar.amber@intel.com>
[...]
> > [Wang, Yipeng] 
> > Hi, Kumar, thanks for the patch.
> > I think the minimum required machine for x86 is sse4.2 compatible already. So I wonder if we really need this.
> 
> Yes, that's why I don't understand this patch.

Any comment? Should we reject the patch?
diff mbox series

Patch

diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
index 3e131aa6bb..e9f063780c 100644
--- a/lib/librte_hash/rte_hash_crc.h
+++ b/lib/librte_hash/rte_hash_crc.h
@@ -358,7 +358,7 @@  crc32c_2words(uint64_t data, uint32_t init_val)
 	return crc;
 }
 
-#if defined(RTE_ARCH_X86)
+#if defined(RTE_ARCH_X86) && defined(__SSE42__)
 static inline uint32_t
 crc32c_sse42_u8(uint8_t data, uint32_t init_val)
 {
@@ -404,7 +404,7 @@  crc32c_sse42_u64_mimic(uint64_t data, uint64_t init_val)
 }
 #endif
 
-#ifdef RTE_ARCH_X86_64
+#if defined(RTE_ARCH_X86_64) && defined(__SSE42__)
 static inline uint32_t
 crc32c_sse42_u64(uint64_t data, uint64_t init_val)
 {
@@ -442,7 +442,7 @@  static uint8_t crc32_alg = CRC32_SW;
 static inline void
 rte_hash_crc_set_alg(uint8_t alg)
 {
-#if defined(RTE_ARCH_X86)
+#if defined(RTE_ARCH_X86) && defined(__SSE42__)
 	if (alg == CRC32_SSE42_x64 &&
 			!rte_cpu_get_flag_enabled(RTE_CPUFLAG_EM64T))
 		alg = CRC32_SSE42;
@@ -471,7 +471,7 @@  RTE_INIT(rte_hash_crc_init_alg)
 static inline uint32_t
 rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
 {
-#if defined RTE_ARCH_X86
+#if defined(RTE_ARCH_X86) && defined(__SSE42__)
 	if (likely(crc32_alg & CRC32_SSE42))
 		return crc32c_sse42_u8(data, init_val);
 #endif
@@ -494,7 +494,7 @@  rte_hash_crc_1byte(uint8_t data, uint32_t init_val)
 static inline uint32_t
 rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
 {
-#if defined RTE_ARCH_X86
+#if defined(RTE_ARCH_X86) && defined(__SSE42__)
 	if (likely(crc32_alg & CRC32_SSE42))
 		return crc32c_sse42_u16(data, init_val);
 #endif
@@ -517,7 +517,7 @@  rte_hash_crc_2byte(uint16_t data, uint32_t init_val)
 static inline uint32_t
 rte_hash_crc_4byte(uint32_t data, uint32_t init_val)
 {
-#if defined RTE_ARCH_X86
+#if defined(RTE_ARCH_X86) && defined(__SSE42__)
 	if (likely(crc32_alg & CRC32_SSE42))
 		return crc32c_sse42_u32(data, init_val);
 #endif
@@ -540,12 +540,12 @@  rte_hash_crc_4byte(uint32_t data, uint32_t init_val)
 static inline uint32_t
 rte_hash_crc_8byte(uint64_t data, uint32_t init_val)
 {
-#ifdef RTE_ARCH_X86_64
+#if defined(RTE_ARCH_X86) && defined(__SSE42__)
 	if (likely(crc32_alg == CRC32_SSE42_x64))
 		return crc32c_sse42_u64(data, init_val);
 #endif
 
-#if defined RTE_ARCH_X86
+#if defined(RTE_ARCH_X86) && defined(__SSE42__)
 	if (likely(crc32_alg & CRC32_SSE42))
 		return crc32c_sse42_u64_mimic(data, init_val);
 #endif