DPDK patches and discussions
 help / color / mirror / Atom feed
* [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way
@ 2024-11-27 22:57 Andre Muezerie
  2024-11-27 22:57 ` [PATCH 2/2] app/test: add test_init_m128i using compiler intrinsic Andre Muezerie
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Andre Muezerie @ 2024-11-27 22:57 UTC (permalink / raw)
  To: Yipeng Wang, Sameh Gobriel, Bruce Richardson, Vladimir Medvedkin
  Cc: dev, Andre Muezerie

The mechanism used to initialize an __m128i data type in rte_thash.h is
non-portable and MSVC does not like it. It clearly is not doing what
is desired:

..\lib\hash\rte_thash.h(38): warning C4305: 'initializing':
    truncation from 'unsigned __int64' to 'char'
..\lib\hash\rte_thash.h(38): warning C4305: 'initializing':
    truncation from 'unsigned __int64' to 'char'

A more portable approach is to use compiler intrinsics to perform the
initialization. This patch uses a single compiler intrinsic to
initialize the data type using a sequence of 16 bytes stored in
memory.

There should be no perf degradation due to this change.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 lib/hash/rte_thash.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lib/hash/rte_thash.h b/lib/hash/rte_thash.h
index c0af5968df..3512639792 100644
--- a/lib/hash/rte_thash.h
+++ b/lib/hash/rte_thash.h
@@ -34,8 +34,9 @@ extern "C" {
 /* Byte swap mask used for converting IPv6 address
  * 4-byte chunks to CPU byte order
  */
-static const __m128i rte_thash_ipv6_bswap_mask = {
-		0x0405060700010203ULL, 0x0C0D0E0F08090A0BULL};
+static const uint8_t rte_thash_ipv6_bswap_mask[] = {
+		0x03, 0x02, 0x01, 0x00, 0x07, 0x06, 0x05, 0x04,
+		0x0B, 0x0A, 0x09, 0x08, 0x0F, 0x0E, 0x0D, 0x0C};
 #endif
 
 /**
@@ -152,12 +153,14 @@ rte_thash_load_v6_addrs(const struct rte_ipv6_hdr *orig,
 			union rte_thash_tuple *targ)
 {
 #ifdef RTE_ARCH_X86
+	const __m128i ipv6_bswap_mask =
+			_mm_loadu_si128((const __m128i*)&rte_thash_ipv6_bswap_mask);
 	__m128i ipv6 = _mm_loadu_si128((const __m128i *)&orig->src_addr);
 	*(__m128i *)&targ->v6.src_addr =
-			_mm_shuffle_epi8(ipv6, rte_thash_ipv6_bswap_mask);
+			_mm_shuffle_epi8(ipv6, ipv6_bswap_mask);
 	ipv6 = _mm_loadu_si128((const __m128i *)&orig->dst_addr);
 	*(__m128i *)&targ->v6.dst_addr =
-			_mm_shuffle_epi8(ipv6, rte_thash_ipv6_bswap_mask);
+			_mm_shuffle_epi8(ipv6, ipv6_bswap_mask);
 #elif defined(__ARM_NEON)
 	uint8x16_t ipv6 = vld1q_u8(orig->src_addr.a);
 	vst1q_u8(targ->v6.src_addr.a, vrev32q_u8(ipv6));
-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH 2/2] app/test: add test_init_m128i using compiler intrinsic
  2024-11-27 22:57 [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way Andre Muezerie
@ 2024-11-27 22:57 ` Andre Muezerie
  2025-03-03 22:29   ` Andre Muezerie
  2025-03-03 22:27 ` [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way Andre Muezerie
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Andre Muezerie @ 2024-11-27 22:57 UTC (permalink / raw)
  To: Yipeng Wang, Sameh Gobriel, Bruce Richardson, Vladimir Medvedkin
  Cc: dev, Andre Muezerie

This test initializes an __m128i data type using the old
non-portable way used until now and the more portable way
using compiler intrinsics. The test ensures the resulting
values after initialization match.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 app/test/test_thash.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/app/test/test_thash.c b/app/test/test_thash.c
index b9c6e9118e..c121b1f43f 100644
--- a/app/test/test_thash.c
+++ b/app/test/test_thash.c
@@ -1030,6 +1030,38 @@ test_keygen(void)
 	return TEST_SUCCESS;
 }
 
+#ifdef RTE_ARCH_X86
+#ifndef RTE_TOOLCHAIN_MSVC
+static int
+test_init_m128i(void)
+{
+	/* When initializing __m128i with two constant values like below
+	 * MSVC issues warning C4305:
+	 *     'initializing': truncation from 'unsigned __int64' to 'char'
+	 */
+	static const __m128i a = {
+			0x0405060700010203ULL, 0x0C0D0E0F08090A0BULL};
+
+	/* Using compiler intrinsics to initialize __m128i is therefore
+	 * preferred, like below
+	 */
+	static const uint8_t b_bytes[] = {
+			0x03, 0x02, 0x01, 0x00, 0x07, 0x06, 0x05, 0x04,
+			0x0B, 0x0A, 0x09, 0x08, 0x0F, 0x0E, 0x0D, 0x0C};
+	const __m128i b =
+			_mm_loadu_si128((const __m128i *)&b_bytes);
+
+	if (memcmp(&a, &b, sizeof(a)) != 0) {
+		printf("Same value was expected when initializing data "
+				"type using compiler intrinsic\n");
+		return -1;
+	}
+
+	return TEST_SUCCESS;
+}
+#endif
+#endif
+
 static struct unit_test_suite thash_tests = {
 	.suite_name = "thash autotest",
 	.setup = NULL,
@@ -1052,6 +1084,11 @@ static struct unit_test_suite thash_tests = {
 	TEST_CASE(test_adjust_tuple),
 	TEST_CASE(test_adjust_tuple_mult_reta),
 	TEST_CASE(test_keygen),
+#ifdef RTE_ARCH_X86
+#ifndef RTE_TOOLCHAIN_MSVC
+	TEST_CASE(test_init_m128i),
+#endif
+#endif
 	TEST_CASES_END()
 	}
 };
-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way
  2024-11-27 22:57 [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way Andre Muezerie
  2024-11-27 22:57 ` [PATCH 2/2] app/test: add test_init_m128i using compiler intrinsic Andre Muezerie
@ 2025-03-03 22:27 ` Andre Muezerie
  2025-03-04 10:46 ` Bruce Richardson
  2025-03-04 21:53 ` [PATCH v2 " Andre Muezerie
  3 siblings, 0 replies; 10+ messages in thread
From: Andre Muezerie @ 2025-03-03 22:27 UTC (permalink / raw)
  To: Yipeng Wang, Sameh Gobriel, Bruce Richardson, Vladimir Medvedkin; +Cc: dev

On Wed, Nov 27, 2024 at 02:57:57PM -0800, Andre Muezerie wrote:
> The mechanism used to initialize an __m128i data type in rte_thash.h is
> non-portable and MSVC does not like it. It clearly is not doing what
> is desired:
> 
> ..\lib\hash\rte_thash.h(38): warning C4305: 'initializing':
>     truncation from 'unsigned __int64' to 'char'
> ..\lib\hash\rte_thash.h(38): warning C4305: 'initializing':
>     truncation from 'unsigned __int64' to 'char'
> 
> A more portable approach is to use compiler intrinsics to perform the
> initialization. This patch uses a single compiler intrinsic to
> initialize the data type using a sequence of 16 bytes stored in
> memory.
> 
> There should be no perf degradation due to this change.
> 
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> ---
>  lib/hash/rte_thash.h | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/hash/rte_thash.h b/lib/hash/rte_thash.h
> index c0af5968df..3512639792 100644
> --- a/lib/hash/rte_thash.h
> +++ b/lib/hash/rte_thash.h
> @@ -34,8 +34,9 @@ extern "C" {
>  /* Byte swap mask used for converting IPv6 address
>   * 4-byte chunks to CPU byte order
>   */
> -static const __m128i rte_thash_ipv6_bswap_mask = {
> -		0x0405060700010203ULL, 0x0C0D0E0F08090A0BULL};
> +static const uint8_t rte_thash_ipv6_bswap_mask[] = {
> +		0x03, 0x02, 0x01, 0x00, 0x07, 0x06, 0x05, 0x04,
> +		0x0B, 0x0A, 0x09, 0x08, 0x0F, 0x0E, 0x0D, 0x0C};
>  #endif
>  
>  /**
> @@ -152,12 +153,14 @@ rte_thash_load_v6_addrs(const struct rte_ipv6_hdr *orig,
>  			union rte_thash_tuple *targ)
>  {
>  #ifdef RTE_ARCH_X86
> +	const __m128i ipv6_bswap_mask =
> +			_mm_loadu_si128((const __m128i*)&rte_thash_ipv6_bswap_mask);
>  	__m128i ipv6 = _mm_loadu_si128((const __m128i *)&orig->src_addr);
>  	*(__m128i *)&targ->v6.src_addr =
> -			_mm_shuffle_epi8(ipv6, rte_thash_ipv6_bswap_mask);
> +			_mm_shuffle_epi8(ipv6, ipv6_bswap_mask);
>  	ipv6 = _mm_loadu_si128((const __m128i *)&orig->dst_addr);
>  	*(__m128i *)&targ->v6.dst_addr =
> -			_mm_shuffle_epi8(ipv6, rte_thash_ipv6_bswap_mask);
> +			_mm_shuffle_epi8(ipv6, ipv6_bswap_mask);
>  #elif defined(__ARM_NEON)
>  	uint8x16_t ipv6 = vld1q_u8(orig->src_addr.a);
>  	vst1q_u8(targ->v6.src_addr.a, vrev32q_u8(ipv6));
> -- 
> 2.34.1

Could someone please review this patch and let me know if there are changes to be made?
I have other patches depending on this.

Thanks,
Andre Muezerie

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] app/test: add test_init_m128i using compiler intrinsic
  2024-11-27 22:57 ` [PATCH 2/2] app/test: add test_init_m128i using compiler intrinsic Andre Muezerie
@ 2025-03-03 22:29   ` Andre Muezerie
  0 siblings, 0 replies; 10+ messages in thread
From: Andre Muezerie @ 2025-03-03 22:29 UTC (permalink / raw)
  To: Yipeng Wang, Sameh Gobriel, Bruce Richardson, Vladimir Medvedkin; +Cc: dev

On Wed, Nov 27, 2024 at 02:57:58PM -0800, Andre Muezerie wrote:
> This test initializes an __m128i data type using the old
> non-portable way used until now and the more portable way
> using compiler intrinsics. The test ensures the resulting
> values after initialization match.
> 
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> ---
>  app/test/test_thash.c | 37 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/app/test/test_thash.c b/app/test/test_thash.c
> index b9c6e9118e..c121b1f43f 100644
> --- a/app/test/test_thash.c
> +++ b/app/test/test_thash.c
> @@ -1030,6 +1030,38 @@ test_keygen(void)
>  	return TEST_SUCCESS;
>  }
>  
> +#ifdef RTE_ARCH_X86
> +#ifndef RTE_TOOLCHAIN_MSVC
> +static int
> +test_init_m128i(void)
> +{
> +	/* When initializing __m128i with two constant values like below
> +	 * MSVC issues warning C4305:
> +	 *     'initializing': truncation from 'unsigned __int64' to 'char'
> +	 */
> +	static const __m128i a = {
> +			0x0405060700010203ULL, 0x0C0D0E0F08090A0BULL};
> +
> +	/* Using compiler intrinsics to initialize __m128i is therefore
> +	 * preferred, like below
> +	 */
> +	static const uint8_t b_bytes[] = {
> +			0x03, 0x02, 0x01, 0x00, 0x07, 0x06, 0x05, 0x04,
> +			0x0B, 0x0A, 0x09, 0x08, 0x0F, 0x0E, 0x0D, 0x0C};
> +	const __m128i b =
> +			_mm_loadu_si128((const __m128i *)&b_bytes);
> +
> +	if (memcmp(&a, &b, sizeof(a)) != 0) {
> +		printf("Same value was expected when initializing data "
> +				"type using compiler intrinsic\n");
> +		return -1;
> +	}
> +
> +	return TEST_SUCCESS;
> +}
> +#endif
> +#endif
> +
>  static struct unit_test_suite thash_tests = {
>  	.suite_name = "thash autotest",
>  	.setup = NULL,
> @@ -1052,6 +1084,11 @@ static struct unit_test_suite thash_tests = {
>  	TEST_CASE(test_adjust_tuple),
>  	TEST_CASE(test_adjust_tuple_mult_reta),
>  	TEST_CASE(test_keygen),
> +#ifdef RTE_ARCH_X86
> +#ifndef RTE_TOOLCHAIN_MSVC
> +	TEST_CASE(test_init_m128i),
> +#endif
> +#endif
>  	TEST_CASES_END()
>  	}
>  };
> -- 
> 2.34.1

Could someone please review this patch and let me know if there are changes to be made?
I have other patches depending on this.

Thanks,
Andre Muezerie


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way
  2024-11-27 22:57 [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way Andre Muezerie
  2024-11-27 22:57 ` [PATCH 2/2] app/test: add test_init_m128i using compiler intrinsic Andre Muezerie
  2025-03-03 22:27 ` [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way Andre Muezerie
@ 2025-03-04 10:46 ` Bruce Richardson
  2025-03-04 21:53 ` [PATCH v2 " Andre Muezerie
  3 siblings, 0 replies; 10+ messages in thread
From: Bruce Richardson @ 2025-03-04 10:46 UTC (permalink / raw)
  To: Andre Muezerie; +Cc: Yipeng Wang, Sameh Gobriel, Vladimir Medvedkin, dev

On Wed, Nov 27, 2024 at 02:57:57PM -0800, Andre Muezerie wrote:
> The mechanism used to initialize an __m128i data type in rte_thash.h is
> non-portable and MSVC does not like it. It clearly is not doing what
> is desired:
> 
> ..\lib\hash\rte_thash.h(38): warning C4305: 'initializing':
>     truncation from 'unsigned __int64' to 'char'
> ..\lib\hash\rte_thash.h(38): warning C4305: 'initializing':
>     truncation from 'unsigned __int64' to 'char'
> 
> A more portable approach is to use compiler intrinsics to perform the
> initialization. This patch uses a single compiler intrinsic to
> initialize the data type using a sequence of 16 bytes stored in
> memory.
> 
> There should be no perf degradation due to this change.
> 
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> ---
>  lib/hash/rte_thash.h | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/hash/rte_thash.h b/lib/hash/rte_thash.h
> index c0af5968df..3512639792 100644
> --- a/lib/hash/rte_thash.h
> +++ b/lib/hash/rte_thash.h
> @@ -34,8 +34,9 @@ extern "C" {
>  /* Byte swap mask used for converting IPv6 address
>   * 4-byte chunks to CPU byte order
>   */
> -static const __m128i rte_thash_ipv6_bswap_mask = {
> -		0x0405060700010203ULL, 0x0C0D0E0F08090A0BULL};

Does work just changing this to using _mm_set_epi64x(), or _mm_set_epi32()? 
If necessary we can just move this constant into the function using it, to
set use those functions instead.

> +static const uint8_t rte_thash_ipv6_bswap_mask[] = {
> +		0x03, 0x02, 0x01, 0x00, 0x07, 0x06, 0x05, 0x04,
> +		0x0B, 0x0A, 0x09, 0x08, 0x0F, 0x0E, 0x0D, 0x0C};
>  #endif
>  
>  /**
> @@ -152,12 +153,14 @@ rte_thash_load_v6_addrs(const struct rte_ipv6_hdr *orig,
>  			union rte_thash_tuple *targ)
>  {
>  #ifdef RTE_ARCH_X86
> +	const __m128i ipv6_bswap_mask =
> +			_mm_loadu_si128((const __m128i*)&rte_thash_ipv6_bswap_mask);
>  	__m128i ipv6 = _mm_loadu_si128((const __m128i *)&orig->src_addr);
>  	*(__m128i *)&targ->v6.src_addr =
> -			_mm_shuffle_epi8(ipv6, rte_thash_ipv6_bswap_mask);
> +			_mm_shuffle_epi8(ipv6, ipv6_bswap_mask);
>  	ipv6 = _mm_loadu_si128((const __m128i *)&orig->dst_addr);
>  	*(__m128i *)&targ->v6.dst_addr =
> -			_mm_shuffle_epi8(ipv6, rte_thash_ipv6_bswap_mask);
> +			_mm_shuffle_epi8(ipv6, ipv6_bswap_mask);
>  #elif defined(__ARM_NEON)
>  	uint8x16_t ipv6 = vld1q_u8(orig->src_addr.a);
>  	vst1q_u8(targ->v6.src_addr.a, vrev32q_u8(ipv6));
> -- 
> 2.34.1
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] lib/hash: initialize __m128i data type in a portable way
  2024-11-27 22:57 [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way Andre Muezerie
                   ` (2 preceding siblings ...)
  2025-03-04 10:46 ` Bruce Richardson
@ 2025-03-04 21:53 ` Andre Muezerie
  2025-03-04 21:53   ` [PATCH v2 2/2] app/test: add test_init_m128i using compiler intrinsic Andre Muezerie
  2025-03-05  9:13   ` [PATCH v2 1/2] lib/hash: initialize __m128i data type in a portable way Bruce Richardson
  3 siblings, 2 replies; 10+ messages in thread
From: Andre Muezerie @ 2025-03-04 21:53 UTC (permalink / raw)
  To: andremue
  Cc: bruce.richardson, dev, sameh.gobriel, vladimir.medvedkin, yipeng1.wang

The mechanism used to initialize an __m128i data type in rte_thash.h is
non-portable and MSVC does not like it. It clearly is not doing what
is desired:

..\lib\hash\rte_thash.h(38): warning C4305: 'initializing':
    truncation from 'unsigned __int64' to 'char'
..\lib\hash\rte_thash.h(38): warning C4305: 'initializing':
    truncation from 'unsigned __int64' to 'char'

A more portable approach is to use compiler intrinsics to perform the
initialization. This patch uses a single compiler intrinsic to
initialize the data.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 lib/hash/rte_thash.h | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/lib/hash/rte_thash.h b/lib/hash/rte_thash.h
index 04f9f1875c..8bca430663 100644
--- a/lib/hash/rte_thash.h
+++ b/lib/hash/rte_thash.h
@@ -30,14 +30,6 @@
 extern "C" {
 #endif
 
-#ifdef RTE_ARCH_X86
-/* Byte swap mask used for converting IPv6 address
- * 4-byte chunks to CPU byte order
- */
-static const __m128i rte_thash_ipv6_bswap_mask = {
-		0x0405060700010203ULL, 0x0C0D0E0F08090A0BULL};
-#endif
-
 /**
  * length in dwords of input tuple to
  * calculate hash of ipv4 header only
@@ -159,6 +151,11 @@ rte_thash_load_v6_addrs(const struct rte_ipv6_hdr *orig,
 			union rte_thash_tuple *targ)
 {
 #ifdef RTE_ARCH_X86
+	/* Byte swap mask used for converting IPv6 address
+	 * 4-byte chunks to CPU byte order
+	 */
+	const __m128i rte_thash_ipv6_bswap_mask = _mm_set_epi64x(
+		0x0C0D0E0F08090A0BULL, 0x0405060700010203ULL);
 	__m128i ipv6 = _mm_loadu_si128((const __m128i *)&orig->src_addr);
 	*(__m128i *)&targ->v6.src_addr =
 			_mm_shuffle_epi8(ipv6, rte_thash_ipv6_bswap_mask);
-- 
2.48.1.vfs.0.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] app/test: add test_init_m128i using compiler intrinsic
  2025-03-04 21:53 ` [PATCH v2 " Andre Muezerie
@ 2025-03-04 21:53   ` Andre Muezerie
  2025-03-05  9:20     ` Bruce Richardson
  2025-03-05  9:13   ` [PATCH v2 1/2] lib/hash: initialize __m128i data type in a portable way Bruce Richardson
  1 sibling, 1 reply; 10+ messages in thread
From: Andre Muezerie @ 2025-03-04 21:53 UTC (permalink / raw)
  To: andremue
  Cc: bruce.richardson, dev, sameh.gobriel, vladimir.medvedkin, yipeng1.wang

This test initializes an __m128i data type using the old
non-portable way used until now and the more portable way
using compiler intrinsics. The test ensures the resulting
values after initialization match.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 app/test/test_thash.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/app/test/test_thash.c b/app/test/test_thash.c
index 33b0c6adac..5f7081a3ad 100644
--- a/app/test/test_thash.c
+++ b/app/test/test_thash.c
@@ -1029,6 +1029,35 @@ test_keygen(void)
 	return TEST_SUCCESS;
 }
 
+#ifdef RTE_ARCH_X86
+#ifndef RTE_TOOLCHAIN_MSVC
+static int
+test_init_m128i(void)
+{
+	/* When initializing __m128i with two constant values like below
+	 * MSVC issues warning C4305:
+	 *     'initializing': truncation from 'unsigned __int64' to 'char'
+	 */
+	static const __m128i a = {
+			0x0405060700010203ULL, 0x0C0D0E0F08090A0BULL};
+
+	/* Using compiler intrinsics to initialize __m128i is therefore
+	 * preferred, like below
+	 */
+	const __m128i b = _mm_set_epi64x(
+			0x0C0D0E0F08090A0BULL, 0x0405060700010203ULL);
+
+	if (memcmp(&a, &b, sizeof(a)) != 0) {
+		printf("Same value was expected when initializing data "
+				"type using compiler intrinsic\n");
+		return -1;
+	}
+
+	return TEST_SUCCESS;
+}
+#endif
+#endif
+
 static struct unit_test_suite thash_tests = {
 	.suite_name = "thash autotest",
 	.setup = NULL,
@@ -1051,6 +1080,11 @@ static struct unit_test_suite thash_tests = {
 	TEST_CASE(test_adjust_tuple),
 	TEST_CASE(test_adjust_tuple_mult_reta),
 	TEST_CASE(test_keygen),
+#ifdef RTE_ARCH_X86
+#ifndef RTE_TOOLCHAIN_MSVC
+	TEST_CASE(test_init_m128i),
+#endif
+#endif
 	TEST_CASES_END()
 	}
 };
-- 
2.48.1.vfs.0.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 1/2] lib/hash: initialize __m128i data type in a portable way
  2025-03-04 21:53 ` [PATCH v2 " Andre Muezerie
  2025-03-04 21:53   ` [PATCH v2 2/2] app/test: add test_init_m128i using compiler intrinsic Andre Muezerie
@ 2025-03-05  9:13   ` Bruce Richardson
  1 sibling, 0 replies; 10+ messages in thread
From: Bruce Richardson @ 2025-03-05  9:13 UTC (permalink / raw)
  To: Andre Muezerie; +Cc: dev, sameh.gobriel, vladimir.medvedkin, yipeng1.wang

On Tue, Mar 04, 2025 at 01:53:18PM -0800, Andre Muezerie wrote:
> The mechanism used to initialize an __m128i data type in rte_thash.h is
> non-portable and MSVC does not like it. It clearly is not doing what
> is desired:
> 
> ..\lib\hash\rte_thash.h(38): warning C4305: 'initializing':
>     truncation from 'unsigned __int64' to 'char'
> ..\lib\hash\rte_thash.h(38): warning C4305: 'initializing':
>     truncation from 'unsigned __int64' to 'char'
> 
> A more portable approach is to use compiler intrinsics to perform the
> initialization. This patch uses a single compiler intrinsic to
> initialize the data.
> 
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> ---
>  lib/hash/rte_thash.h | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
Much simpler fix, thanks.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] app/test: add test_init_m128i using compiler intrinsic
  2025-03-04 21:53   ` [PATCH v2 2/2] app/test: add test_init_m128i using compiler intrinsic Andre Muezerie
@ 2025-03-05  9:20     ` Bruce Richardson
  2025-03-05 14:50       ` Andre Muezerie
  0 siblings, 1 reply; 10+ messages in thread
From: Bruce Richardson @ 2025-03-05  9:20 UTC (permalink / raw)
  To: Andre Muezerie; +Cc: dev, sameh.gobriel, vladimir.medvedkin, yipeng1.wang

On Tue, Mar 04, 2025 at 01:53:19PM -0800, Andre Muezerie wrote:
> This test initializes an __m128i data type using the old
> non-portable way used until now and the more portable way
> using compiler intrinsics. The test ensures the resulting
> values after initialization match.
> 
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> ---
>  app/test/test_thash.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/app/test/test_thash.c b/app/test/test_thash.c
> index 33b0c6adac..5f7081a3ad 100644
> --- a/app/test/test_thash.c
> +++ b/app/test/test_thash.c
> @@ -1029,6 +1029,35 @@ test_keygen(void)
>  	return TEST_SUCCESS;
>  }
>  
> +#ifdef RTE_ARCH_X86
> +#ifndef RTE_TOOLCHAIN_MSVC
> +static int
> +test_init_m128i(void)
> +{
> +	/* When initializing __m128i with two constant values like below
> +	 * MSVC issues warning C4305:
> +	 *     'initializing': truncation from 'unsigned __int64' to 'char'
> +	 */
> +	static const __m128i a = {
> +			0x0405060700010203ULL, 0x0C0D0E0F08090A0BULL};
> +
> +	/* Using compiler intrinsics to initialize __m128i is therefore
> +	 * preferred, like below
> +	 */
> +	const __m128i b = _mm_set_epi64x(
> +			0x0C0D0E0F08090A0BULL, 0x0405060700010203ULL);
> +
> +	if (memcmp(&a, &b, sizeof(a)) != 0) {
> +		printf("Same value was expected when initializing data "
> +				"type using compiler intrinsic\n");
> +		return -1;
> +	}
> +
> +	return TEST_SUCCESS;
> +}
> +#endif
> +#endif
> +
Do we still need this patch? I don't think its necessary.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] app/test: add test_init_m128i using compiler intrinsic
  2025-03-05  9:20     ` Bruce Richardson
@ 2025-03-05 14:50       ` Andre Muezerie
  0 siblings, 0 replies; 10+ messages in thread
From: Andre Muezerie @ 2025-03-05 14:50 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev, sameh.gobriel, vladimir.medvedkin, yipeng1.wang

On Wed, Mar 05, 2025 at 09:20:03AM +0000, Bruce Richardson wrote:
> On Tue, Mar 04, 2025 at 01:53:19PM -0800, Andre Muezerie wrote:
> > This test initializes an __m128i data type using the old
> > non-portable way used until now and the more portable way
> > using compiler intrinsics. The test ensures the resulting
> > values after initialization match.
> > 
> > Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> > ---
> >  app/test/test_thash.c | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> > 
> > diff --git a/app/test/test_thash.c b/app/test/test_thash.c
> > index 33b0c6adac..5f7081a3ad 100644
> > --- a/app/test/test_thash.c
> > +++ b/app/test/test_thash.c
> > @@ -1029,6 +1029,35 @@ test_keygen(void)
> >  	return TEST_SUCCESS;
> >  }
> >  
> > +#ifdef RTE_ARCH_X86
> > +#ifndef RTE_TOOLCHAIN_MSVC
> > +static int
> > +test_init_m128i(void)
> > +{
> > +	/* When initializing __m128i with two constant values like below
> > +	 * MSVC issues warning C4305:
> > +	 *     'initializing': truncation from 'unsigned __int64' to 'char'
> > +	 */
> > +	static const __m128i a = {
> > +			0x0405060700010203ULL, 0x0C0D0E0F08090A0BULL};
> > +
> > +	/* Using compiler intrinsics to initialize __m128i is therefore
> > +	 * preferred, like below
> > +	 */
> > +	const __m128i b = _mm_set_epi64x(
> > +			0x0C0D0E0F08090A0BULL, 0x0405060700010203ULL);
> > +
> > +	if (memcmp(&a, &b, sizeof(a)) != 0) {
> > +		printf("Same value was expected when initializing data "
> > +				"type using compiler intrinsic\n");
> > +		return -1;
> > +	}
> > +
> > +	return TEST_SUCCESS;
> > +}
> > +#endif
> > +#endif
> > +
> Do we still need this patch? I don't think its necessary.

It was important to give me confidence that I was flipping the arguments correctly. I
agree that moving forward this test does not add much value and can be removed.

What is the correct process to do that? Should I send a new series without that
patch or can it be simply ignored during merge?

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-03-05 14:50 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-11-27 22:57 [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way Andre Muezerie
2024-11-27 22:57 ` [PATCH 2/2] app/test: add test_init_m128i using compiler intrinsic Andre Muezerie
2025-03-03 22:29   ` Andre Muezerie
2025-03-03 22:27 ` [PATCH 1/2] lib/hash: initialize __m128i data type in a portable way Andre Muezerie
2025-03-04 10:46 ` Bruce Richardson
2025-03-04 21:53 ` [PATCH v2 " Andre Muezerie
2025-03-04 21:53   ` [PATCH v2 2/2] app/test: add test_init_m128i using compiler intrinsic Andre Muezerie
2025-03-05  9:20     ` Bruce Richardson
2025-03-05 14:50       ` Andre Muezerie
2025-03-05  9:13   ` [PATCH v2 1/2] lib/hash: initialize __m128i data type in a portable way Bruce Richardson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).