diff mbox

[dpdk-dev,1/3,v2] librte_hash: Fix unsupported instruction `crc32' in i686 platform

Message ID 1425574530-16019-1-git-send-email-michael.qiu@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Michael Qiu March 5, 2015, 4:55 p.m. UTC 
  CC rte_hash.o
Error: unsupported instruction `crc32'

The root cause is that i686 platform does not support 'crc32q'
Need make it only available in x86_64 platform

Signed-off-by: Michael Qiu <michael.qiu@intel.com>
---
v2 --> v1:
         Make crc32 instruction only works in X86 platform
 lib/librte_hash/rte_hash_crc.h | 12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Yerden Zhumabekov March 5, 2015, 5:02 p.m. UTC | #1 
Acked-by: Yerden Zhumabekov <yerden.zhumabekov@sts.kz>

05.03.2015 22:55, Michael Qiu пишет:
> CC rte_hash.o
> Error: unsupported instruction `crc32'
>
> The root cause is that i686 platform does not support 'crc32q'
> Need make it only available in x86_64 platform
>
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
> v2 --> v1:
>          Make crc32 instruction only works in X86 platform
>  lib/librte_hash/rte_hash_crc.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
> index d28bb2a..c0a789e 100644
> --- a/lib/librte_hash/rte_hash_crc.h
> +++ b/lib/librte_hash/rte_hash_crc.h
> @@ -364,6 +364,7 @@ crc32c_2words(uint64_t data, uint32_t init_val)
>  	return crc;
>  }
>  
> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>  static inline uint32_t
>  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>  {
> @@ -373,7 +374,9 @@ crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>  			: [data] "rm" (data));
>  	return init_val;
>  }
> +#endif
>  
> +#ifdef RTE_ARCH_X86_64
>  static inline uint32_t
>  crc32c_sse42_u64(uint64_t data, uint64_t init_val)
>  {
> @@ -383,7 +386,9 @@ crc32c_sse42_u64(uint64_t data, uint64_t init_val)
>  			: [data] "rm" (data));
>  	return init_val;
>  }
> +#endif
>  
> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>  static inline uint32_t
>  crc32c_sse42_u64_mimic(uint64_t data, uint64_t init_val)
>  {
> @@ -397,6 +402,7 @@ crc32c_sse42_u64_mimic(uint64_t data, uint64_t init_val)
>  	init_val = crc32c_sse42_u32(d.u32[1], init_val);
>  	return init_val;
>  }
> +#endif
>  
>  #define CRC32_SW            (1U << 0)
>  #define CRC32_SSE42         (1U << 1)
> @@ -455,8 +461,10 @@ rte_hash_crc_init_alg(void)
>  static inline uint32_t
>  rte_hash_crc_4byte(uint32_t data, uint32_t init_val)
>  {
> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>  	if (likely(crc32_alg & CRC32_SSE42))
>  		return crc32c_sse42_u32(data, init_val);
> +#endif
>  
>  	return crc32c_1word(data, init_val);
>  }
> @@ -476,11 +484,15 @@ 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 (likely(crc32_alg == CRC32_SSE42_x64))
>  		return crc32c_sse42_u64(data, init_val);
> +#endif
>  
> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>  	if (likely(crc32_alg & CRC32_SSE42))
>  		return crc32c_sse42_u64_mimic(data, init_val);
> +#endif
>  
>  	return crc32c_2words(data, init_val);
>  }
Thomas Monjalon March 5, 2015, 5:10 p.m. UTC | #2 
2015-03-06 00:55, Michael Qiu:
> CC rte_hash.o
> Error: unsupported instruction `crc32'
> 
> The root cause is that i686 platform does not support 'crc32q'
> Need make it only available in x86_64 platform
> 
> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> ---
> v2 --> v1:
>          Make crc32 instruction only works in X86 platform
>  lib/librte_hash/rte_hash_crc.h | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
> index d28bb2a..c0a789e 100644
> --- a/lib/librte_hash/rte_hash_crc.h
> +++ b/lib/librte_hash/rte_hash_crc.h
> @@ -364,6 +364,7 @@ crc32c_2words(uint64_t data, uint32_t init_val)
>  	return crc;
>  }
>  
> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>  static inline uint32_t
>  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>  {
> @@ -373,7 +374,9 @@ crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>  			: [data] "rm" (data));
>  	return init_val;
>  }
> +#endif

Wouldn't it be more elegant to define a stub which returns 0 in #else
in order to remove #ifdef below?
Not sure, matter of taste.

[...]
> @@ -455,8 +461,10 @@ rte_hash_crc_init_alg(void)
>  static inline uint32_t
>  rte_hash_crc_4byte(uint32_t data, uint32_t init_val)
>  {
> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>  	if (likely(crc32_alg & CRC32_SSE42))
>  		return crc32c_sse42_u32(data, init_val);
> +#endif
>  
>  	return crc32c_1word(data, init_val);
>  }
> @@ -476,11 +484,15 @@ 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 (likely(crc32_alg == CRC32_SSE42_x64))
>  		return crc32c_sse42_u64(data, init_val);
> +#endif
>  
> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>  	if (likely(crc32_alg & CRC32_SSE42))
>  		return crc32c_sse42_u64_mimic(data, init_val);
> +#endif
>  
>  	return crc32c_2words(data, init_val);
>  }
>
Michael Qiu March 6, 2015, 1:39 a.m. UTC | #3 
On 3/6/2015 1:11 AM, Thomas Monjalon wrote:
> 2015-03-06 00:55, Michael Qiu:
>> CC rte_hash.o
>> Error: unsupported instruction `crc32'
>>
>> The root cause is that i686 platform does not support 'crc32q'
>> Need make it only available in x86_64 platform
>>
>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>> ---
>> v2 --> v1:
>>          Make crc32 instruction only works in X86 platform
>>  lib/librte_hash/rte_hash_crc.h | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
>> index d28bb2a..c0a789e 100644
>> --- a/lib/librte_hash/rte_hash_crc.h
>> +++ b/lib/librte_hash/rte_hash_crc.h
>> @@ -364,6 +364,7 @@ crc32c_2words(uint64_t data, uint32_t init_val)
>>  	return crc;
>>  }
>>  
>> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>>  static inline uint32_t
>>  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>>  {
>> @@ -373,7 +374,9 @@ crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>>  			: [data] "rm" (data));
>>  	return init_val;
>>  }
>> +#endif
> Wouldn't it be more elegant to define a stub which returns 0 in #else
> in order to remove #ifdef below?
> Not sure, matter of taste.

It may be not a good idea, see rte_hash_crc_8byte(), if no crc32
support, it will use crc32c_2words(), if we define a stub which returns
0 in #else, then we need always check the return value whether it is
none-zero otherwise need fallback.

Thanks,
Michael
> [...]
>> @@ -455,8 +461,10 @@ rte_hash_crc_init_alg(void)
>>  static inline uint32_t
>>  rte_hash_crc_4byte(uint32_t data, uint32_t init_val)
>>  {
>> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>>  	if (likely(crc32_alg & CRC32_SSE42))
>>  		return crc32c_sse42_u32(data, init_val);
>> +#endif
>>  
>>  	return crc32c_1word(data, init_val);
>>  }
>> @@ -476,11 +484,15 @@ 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 (likely(crc32_alg == CRC32_SSE42_x64))
>>  		return crc32c_sse42_u64(data, init_val);
>> +#endif
>>  
>> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>>  	if (likely(crc32_alg & CRC32_SSE42))
>>  		return crc32c_sse42_u64_mimic(data, init_val);
>> +#endif
>>  
>>  	return crc32c_2words(data, init_val);
>>  }
>>
>
>
Thomas Monjalon March 7, 2015, 6:39 p.m. UTC | #4 
2015-03-06 01:39, Qiu, Michael:
> On 3/6/2015 1:11 AM, Thomas Monjalon wrote:
> > 2015-03-06 00:55, Michael Qiu:
> >> CC rte_hash.o
> >> Error: unsupported instruction `crc32'
> >>
> >> The root cause is that i686 platform does not support 'crc32q'
> >> Need make it only available in x86_64 platform
> >>
> >> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> >> ---
> >> v2 --> v1:
> >>          Make crc32 instruction only works in X86 platform
> >>  lib/librte_hash/rte_hash_crc.h | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
> >> index d28bb2a..c0a789e 100644
> >> --- a/lib/librte_hash/rte_hash_crc.h
> >> +++ b/lib/librte_hash/rte_hash_crc.h
> >> @@ -364,6 +364,7 @@ crc32c_2words(uint64_t data, uint32_t init_val)
> >>  	return crc;
> >>  }
> >>  
> >> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
> >>  static inline uint32_t
> >>  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
> >>  {
> >> @@ -373,7 +374,9 @@ crc32c_sse42_u32(uint32_t data, uint32_t init_val)
> >>  			: [data] "rm" (data));
> >>  	return init_val;
> >>  }
> >> +#endif
> > 
> > Wouldn't it be more elegant to define a stub which returns 0 in #else
> > in order to remove #ifdef below?
> > Not sure, matter of taste.
> 
> It may be not a good idea, see rte_hash_crc_8byte(), if no crc32
> support, it will use crc32c_2words(), if we define a stub which returns
> 0 in #else, then we need always check the return value whether it is
> none-zero otherwise need fallback.

I don't think so.
The stub won't never been called because they are protected by the cpuflag
condition.
Michael Qiu March 9, 2015, 5:21 a.m. UTC | #5 
On 3/8/2015 2:39 AM, Thomas Monjalon wrote:
> 2015-03-06 01:39, Qiu, Michael:
>> On 3/6/2015 1:11 AM, Thomas Monjalon wrote:
>>> 2015-03-06 00:55, Michael Qiu:
>>>> CC rte_hash.o
>>>> Error: unsupported instruction `crc32'
>>>>
>>>> The root cause is that i686 platform does not support 'crc32q'
>>>> Need make it only available in x86_64 platform
>>>>
>>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>>>> ---
>>>> v2 --> v1:
>>>>          Make crc32 instruction only works in X86 platform
>>>>  lib/librte_hash/rte_hash_crc.h | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
>>>> index d28bb2a..c0a789e 100644
>>>> --- a/lib/librte_hash/rte_hash_crc.h
>>>> +++ b/lib/librte_hash/rte_hash_crc.h
>>>> @@ -364,6 +364,7 @@ crc32c_2words(uint64_t data, uint32_t init_val)
>>>>  	return crc;
>>>>  }
>>>>  
>>>> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>>>>  static inline uint32_t
>>>>  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>>>>  {
>>>> @@ -373,7 +374,9 @@ crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>>>>  			: [data] "rm" (data));
>>>>  	return init_val;
>>>>  }
>>>> +#endif
>>> Wouldn't it be more elegant to define a stub which returns 0 in #else
>>> in order to remove #ifdef below?
>>> Not sure, matter of taste.
>> It may be not a good idea, see rte_hash_crc_8byte(), if no crc32
>> support, it will use crc32c_2words(), if we define a stub which returns
>> 0 in #else, then we need always check the return value whether it is
>> none-zero otherwise need fallback.
> I don't think so.
> The stub won't never been called because they are protected by the cpuflag
> condition.

Hi, Thomas

You are right, I will send out v3 patch to fix this

Thanks,
Michael

>
Yerden Zhumabekov March 10, 2015, 3:55 a.m. UTC | #6 
08.03.2015 0:39, Thomas Monjalon пишет:
> 2015-03-06 01:39, Qiu, Michael:
>> On 3/6/2015 1:11 AM, Thomas Monjalon wrote:
>>> 2015-03-06 00:55, Michael Qiu:
>>>> ... skipped ...
>>>>  
>>>> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>>>>  static inline uint32_t
>>>>  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>>>>  {
>>>> @@ -373,7 +374,9 @@ crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>>>>  			: [data] "rm" (data));
>>>>  	return init_val;
>>>>  }
>>>> +#endif
>>> Wouldn't it be more elegant to define a stub which returns 0 in #else
>>> in order to remove #ifdef below?
>>> Not sure, matter of taste.
>> It may be not a good idea, see rte_hash_crc_8byte(), if no crc32
>> support, it will use crc32c_2words(), if we define a stub which returns
>> 0 in #else, then we need always check the return value whether it is
>> none-zero otherwise need fallback.
> I don't think so.
> The stub won't never been called because they are protected by the cpuflag
> condition.

That would be a bad surprise if one tries to launch that pre-built
binary on SSE4.2-capable arch :) It's fine though, if binary portability
is out of scope here.
Michael Qiu March 19, 2015, 2 a.m. UTC | #7 
On 3/8/2015 2:39 AM, Thomas Monjalon wrote:
> 2015-03-06 01:39, Qiu, Michael:
>> On 3/6/2015 1:11 AM, Thomas Monjalon wrote:
>>> 2015-03-06 00:55, Michael Qiu:
>>>> CC rte_hash.o
>>>> Error: unsupported instruction `crc32'
>>>>
>>>> The root cause is that i686 platform does not support 'crc32q'
>>>> Need make it only available in x86_64 platform
>>>>
>>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
>>>> ---
>>>> v2 --> v1:
>>>>          Make crc32 instruction only works in X86 platform
>>>>  lib/librte_hash/rte_hash_crc.h | 12 ++++++++++++
>>>>  1 file changed, 12 insertions(+)
>>>>
>>>> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
>>>> index d28bb2a..c0a789e 100644
>>>> --- a/lib/librte_hash/rte_hash_crc.h
>>>> +++ b/lib/librte_hash/rte_hash_crc.h
>>>> @@ -364,6 +364,7 @@ crc32c_2words(uint64_t data, uint32_t init_val)
>>>>  	return crc;
>>>>  }
>>>>  
>>>> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
>>>>  static inline uint32_t
>>>>  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>>>>  {
>>>> @@ -373,7 +374,9 @@ crc32c_sse42_u32(uint32_t data, uint32_t init_val)
>>>>  			: [data] "rm" (data));
>>>>  	return init_val;
>>>>  }
>>>> +#endif
>>> Wouldn't it be more elegant to define a stub which returns 0 in #else
>>> in order to remove #ifdef below?
>>> Not sure, matter of taste.
>> It may be not a good idea, see rte_hash_crc_8byte(), if no crc32
>> support, it will use crc32c_2words(), if we define a stub which returns
>> 0 in #else, then we need always check the return value whether it is
>> none-zero otherwise need fallback.
> I don't think so.
> The stub won't never been called because they are protected by the cpuflag
> condition.

Hi, Thomas

Indeed what I concerned occurs, when compile target i686 in X86_64
platform, it will call the stub function

Then, should I make a patch to fix, change the code as this one(V2)

Another choice is you could revert V3 and merge V2.

Or if have other solution?

Thanks,
Michael
>
Thomas Monjalon March 19, 2015, 8:10 a.m. UTC | #8 
2015-03-19 02:00, Qiu, Michael:
> On 3/8/2015 2:39 AM, Thomas Monjalon wrote:
> > 2015-03-06 01:39, Qiu, Michael:
> >> On 3/6/2015 1:11 AM, Thomas Monjalon wrote:
> >>> 2015-03-06 00:55, Michael Qiu:
> >>>> CC rte_hash.o
> >>>> Error: unsupported instruction `crc32'
> >>>>
> >>>> The root cause is that i686 platform does not support 'crc32q'
> >>>> Need make it only available in x86_64 platform
> >>>>
> >>>> Signed-off-by: Michael Qiu <michael.qiu@intel.com>
> >>>> ---
> >>>> v2 --> v1:
> >>>>          Make crc32 instruction only works in X86 platform
> >>>>  lib/librte_hash/rte_hash_crc.h | 12 ++++++++++++
> >>>>  1 file changed, 12 insertions(+)
> >>>>
> >>>> diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
> >>>> index d28bb2a..c0a789e 100644
> >>>> --- a/lib/librte_hash/rte_hash_crc.h
> >>>> +++ b/lib/librte_hash/rte_hash_crc.h
> >>>> @@ -364,6 +364,7 @@ crc32c_2words(uint64_t data, uint32_t init_val)
> >>>>  	return crc;
> >>>>  }
> >>>>  
> >>>> +#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
> >>>>  static inline uint32_t
> >>>>  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
> >>>>  {
> >>>> @@ -373,7 +374,9 @@ crc32c_sse42_u32(uint32_t data, uint32_t init_val)
> >>>>  			: [data] "rm" (data));
> >>>>  	return init_val;
> >>>>  }
> >>>> +#endif
> >>> Wouldn't it be more elegant to define a stub which returns 0 in #else
> >>> in order to remove #ifdef below?
> >>> Not sure, matter of taste.
> >> It may be not a good idea, see rte_hash_crc_8byte(), if no crc32
> >> support, it will use crc32c_2words(), if we define a stub which returns
> >> 0 in #else, then we need always check the return value whether it is
> >> none-zero otherwise need fallback.
> > I don't think so.
> > The stub won't never been called because they are protected by the cpuflag
> > condition.
> 
> Hi, Thomas
> 
> Indeed what I concerned occurs, when compile target i686 in X86_64
> platform, it will call the stub function

Oh, you mean compiling for i686 and run it in x86_64 target,
so the stub for crc32c_sse42_u64 is called?

> Then, should I make a patch to fix, change the code as this one(V2)
> Another choice is you could revert V3 and merge V2.
> Or if have other solution?

It's probably saner to submit a fix you have tested with ifdef like in v2.
Thanks
diff mbox

Patch

diff --git a/lib/librte_hash/rte_hash_crc.h b/lib/librte_hash/rte_hash_crc.h
index d28bb2a..c0a789e 100644
--- a/lib/librte_hash/rte_hash_crc.h
+++ b/lib/librte_hash/rte_hash_crc.h
@@ -364,6 +364,7 @@  crc32c_2words(uint64_t data, uint32_t init_val)
 	return crc;
 }
 
+#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
 static inline uint32_t
 crc32c_sse42_u32(uint32_t data, uint32_t init_val)
 {
@@ -373,7 +374,9 @@  crc32c_sse42_u32(uint32_t data, uint32_t init_val)
 			: [data] "rm" (data));
 	return init_val;
 }
+#endif
 
+#ifdef RTE_ARCH_X86_64
 static inline uint32_t
 crc32c_sse42_u64(uint64_t data, uint64_t init_val)
 {
@@ -383,7 +386,9 @@  crc32c_sse42_u64(uint64_t data, uint64_t init_val)
 			: [data] "rm" (data));
 	return init_val;
 }
+#endif
 
+#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
 static inline uint32_t
 crc32c_sse42_u64_mimic(uint64_t data, uint64_t init_val)
 {
@@ -397,6 +402,7 @@  crc32c_sse42_u64_mimic(uint64_t data, uint64_t init_val)
 	init_val = crc32c_sse42_u32(d.u32[1], init_val);
 	return init_val;
 }
+#endif
 
 #define CRC32_SW            (1U << 0)
 #define CRC32_SSE42         (1U << 1)
@@ -455,8 +461,10 @@  rte_hash_crc_init_alg(void)
 static inline uint32_t
 rte_hash_crc_4byte(uint32_t data, uint32_t init_val)
 {
+#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
 	if (likely(crc32_alg & CRC32_SSE42))
 		return crc32c_sse42_u32(data, init_val);
+#endif
 
 	return crc32c_1word(data, init_val);
 }
@@ -476,11 +484,15 @@  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 (likely(crc32_alg == CRC32_SSE42_x64))
 		return crc32c_sse42_u64(data, init_val);
+#endif
 
+#if defined RTE_ARCH_I686 || defined RTE_ARCH_X86_64
 	if (likely(crc32_alg & CRC32_SSE42))
 		return crc32c_sse42_u64_mimic(data, init_val);
+#endif
 
 	return crc32c_2words(data, init_val);
 }