lib/hash: initialize __m128i data type in a portable way

PATCH Superseded Andre Muezerie 3 participants 11 messages Nov 27, 2024 – Mar 6, 2025

Share & tools

Andre Muezerie

author

2024-11-27 22:58 UTC #0 lore raw

PATCH 1/2 lib/hash: initialize __m128i data type in a portable way

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 <[email protected]>

---
 lib/hash/rte_thash.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

lib/hash/rte_thash.h +7 -4

index c0af5968df..3512639792 100644
@@ -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));

Signature · 1 line

2.34.1

Andre Muezerie

author

2024-11-27 22:58 UTC #1 lore raw

PATCH 2/2 app/test: add test_init_m128i using compiler intrinsic

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 <[email protected]>

---
 app/test/test_thash.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

app/test/test_thash.c +37 -0

index b9c6e9118e..c121b1f43f 100644
@@ -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()
 	}
 };

Signature · 1 line

2.34.1

Andre Muezerie

author

2025-03-03 22:29 UTC #2 lore raw

Re: [PATCH 2/2] app/test: add test_init_m128i using compiler intrinsic

Quoted 67 lines · Andre Muezerie

Signed-off-by Andre Muezerie <[email protected]>

---
 app/test/test_thash.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

app/test/test_thash.c +37 -0

index b9c6e9118e..c121b1f43f 100644
@@ -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()
 	}
 };

Signature · 1 line

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

Andre Muezerie

author

2025-03-03 22:27 UTC #3 lore raw

In reply to Andre Muezerie

Quoted 56 lines · 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 <[email protected]>
---
 lib/hash/rte_thash.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
lib/hash/rte_thash.h +7 -4
index c0af5968df..3512639792 100644
@@ -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));
Signature · 1 line
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

Bruce Richardson

2025-03-04 10:47 UTC #4 lore raw

In reply to Andre Muezerie

Quoted 26 lines · 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 <[email protected]>
---
 lib/hash/rte_thash.h | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
lib/hash/rte_thash.h +0 -0
index c0af5968df..3512639792 100644

lib/hash/rte_thash.h extern "C" { · line 38

Quoted 5 lines

lib/hash/rte_thash.h +0 -2

@@ -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.

Quoted 26 lines

lib/hash/rte_thash.h +7 -2

+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));

Signature · 1 line

2.34.1

Andre Muezerie

author

2025-03-04 21:53 UTC #5 lore raw

PATCH v2 1/2 lib/hash: initialize __m128i data type in a portable way

In reply to 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:

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 <[email protected]>

---
 lib/hash/rte_thash.h | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

lib/hash/rte_thash.h +5 -8

index 04f9f1875c..8bca430663 100644
@@ -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);

Signature · 1 line

2.48.1.vfs.0.0

Andre Muezerie

author

2025-03-04 21:53 UTC #6 lore raw

PATCH v2 2/2 app/test: add test_init_m128i using compiler intrinsic

Signed-off-by Andre Muezerie <[email protected]>

---
 app/test/test_thash.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

app/test/test_thash.c +34 -0

index 33b0c6adac..5f7081a3ad 100644
@@ -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()
 	}
 };

Signature · 1 line

2.48.1.vfs.0.0

Bruce Richardson

2025-03-05 09:20 UTC #7 lore raw

Re: [PATCH v2 2/2] app/test: add test_init_m128i using compiler intrinsic

Quoted 37 lines · Andre Muezerie

Signed-off-by Andre Muezerie <[email protected]>

---
 app/test/test_thash.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

app/test/test_thash.c +19 -0

index 33b0c6adac..5f7081a3ad 100644
@@ -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) {

app/test/test_thash.c test_keygen(void) · line 1060

Quoted 10 lines

app/test/test_thash.c +10 -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.

Andre Muezerie

author

2025-03-05 14:50 UTC #8 lore raw

Re: [PATCH v2 2/2] app/test: add test_init_m128i using compiler intrinsic

Quoted 41 lines · Bruce Richardson

Quoted 40 lines · Andre Muezerie

Signed-off-by Andre Muezerie <[email protected]>

---
 app/test/test_thash.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

app/test/test_thash.c +22 -0

index 33b0c6adac..5f7081a3ad 100644
@@ -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;

Quoted 8 lines

app/test/test_thash.c test_keygen(void) · line 1060
Quoted 7 lines
app/test/test_thash.c +7 -0
+	}
+
+	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?

Bruce Richardson

Acked-by on v2 1/2

2025-03-05 09:13 UTC #9 lore raw

In reply to Andre Muezerie

Quoted 9 lines · 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'

Quoted 9 lines

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 <[email protected]>
---
 lib/hash/rte_thash.h | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

Much simpler fix, thanks.

Acked-by Bruce Richardson <[email protected]>

David Marchand

Applied

2025-03-06 13:34 UTC #10 lore raw

In reply to Andre Muezerie

Quoted 10 lines · 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'

Quoted 5 lines

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 <[email protected]>

No need to send a new revision.
I applied just this patch, and dropped the second patch in patchwork.

Applied, thanks.

Signature · 1 line

David Marchand