diff mbox series

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

Message ID 1741125199-1217-2-git-send-email-andremue@linux.microsoft.com (mailing list archive)
State Rejected, archived
Delegated to: David Marchand
Headers
Series [v2,1/2] lib/hash: initialize __m128i data type in a portable way |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-marvell-Functional success Functional Testing PASS
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/github-robot: build success github build: passed
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS

Commit Message

Andre Muezerie March 4, 2025, 9:53 p.m. UTC 
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(+)

Comments

Bruce Richardson March 5, 2025, 9:20 a.m. UTC | #1 
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.
Andre Muezerie March 5, 2025, 2:50 p.m. UTC | #2 
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?
diff mbox series

Patch

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()
 	}
 };