[PATCH 0/2] provide rte_ffs32, rte_ffs64 and __rte_x86_movdiri for MSVC

[PATCH 0/2] provide rte_ffs32, rte_ffs64 and __rte_x86_movdiri for MSVC

[Tyler Retzlaff]

MSVC does not support inline assembly so use movdiri intrinsic and
provide abstracted rte_ffs{32,64} inline functions instead of directly
using GCC built-ins.

Tyler Retzlaff (2):
  eal: provide movdiri for MSVC
  eal: add rte ffs32 and rte ffs64 inline functions

 lib/eal/include/rte_bitops.h | 34 ++++++++++++++++++++++++++++++++++
 lib/eal/x86/include/rte_io.h |  4 ++++
 2 files changed, 38 insertions(+)

-- 
1.8.3.1

[PATCH 1/2] eal: provide movdiri for MSVC

[Tyler Retzlaff]

MSVC does not support inline assembly instead it provides compiler
intrinsics. Provide conditional compile for MSVC for movdiri using the
_directstoreu_u32 intrinsic.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/x86/include/rte_io.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/eal/x86/include/rte_io.h b/lib/eal/x86/include/rte_io.h
index 0e1fefd..2c72601 100644
--- a/lib/eal/x86/include/rte_io.h
+++ b/lib/eal/x86/include/rte_io.h
@@ -22,11 +22,15 @@
 static __rte_always_inline void
 __rte_x86_movdiri(uint32_t value, volatile void *addr)
 {
+#ifdef RTE_TOOLCHAIN_MSVC
+	_directstoreu_u32((void *)(uintptr_t)addr, value);
+#else
 	asm volatile(
 		/* MOVDIRI */
 		".byte 0x40, 0x0f, 0x38, 0xf9, 0x02"
 		:
 		: "a" (value), "d" (addr));
+#endif
 }
 
 __rte_experimental
-- 
1.8.3.1

[PATCH 2/2] eal: add rte ffs32 and rte ffs64 inline functions

[Tyler Retzlaff]

provide toolchain abstraction for __builtin_ffs{,l,ll} gcc built-in
intrinsics.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/rte_bitops.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
index 449565e..e157a45 100644
--- a/lib/eal/include/rte_bitops.h
+++ b/lib/eal/include/rte_bitops.h
@@ -405,6 +405,28 @@
 	return (unsigned int)__popcnt64(v);
 }
 
+static inline unsigned int
+rte_ffs32(uint32_t v)
+{
+	unsigned long rv;
+
+	if (0 == _BitScanForward(&rv, v))
+		return 0;
+
+	return (unsigned int)rv + 1;
+}
+
+static inline unsigned int
+rte_ffs64(uint64_t v)
+{
+	unsigned long rv;
+
+	if (0 == _BitScanForward64(&rv, v))
+		return 0;
+
+	return (unsigned int)rv + 1;
+}
+
 #else
 
 /**
@@ -491,6 +513,18 @@
 	return (unsigned int)__builtin_popcountll(v);
 }
 
+static inline unsigned int
+rte_ffs32(uint32_t v)
+{
+	return (unsigned int)__builtin_ffs(v);
+}
+
+static inline unsigned int
+rte_ffs64(uint64_t v)
+{
+	return (unsigned int)__builtin_ffsll(v);
+}
+
 #endif
 
 /**
-- 
1.8.3.1

Re: [PATCH 2/2] eal: add rte ffs32 and rte ffs64 inline functions

[David Marchand]

On Wed, Mar 20, 2024 at 10:25 PM Tyler Retzlaff
<roretzla@linux.microsoft.com> wrote:
>
> provide toolchain abstraction for __builtin_ffs{,l,ll} gcc built-in
> intrinsics.
>
> Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>

Please add a unit test and an entry in the release notes.

> ---
>  lib/eal/include/rte_bitops.h | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
> index 449565e..e157a45 100644
> --- a/lib/eal/include/rte_bitops.h
> +++ b/lib/eal/include/rte_bitops.h
> @@ -405,6 +405,28 @@
>         return (unsigned int)__popcnt64(v);
>  }
>
> +static inline unsigned int
> +rte_ffs32(uint32_t v)
> +{
> +       unsigned long rv;
> +
> +       if (0 == _BitScanForward(&rv, v))

_BitScanForward(&rv, v) == 0 please.


-- 
David Marchand

Re: [PATCH 2/2] eal: add rte ffs32 and rte ffs64 inline functions

[Andre Muezerie]

On Fri, Jun 14, 2024 at 04:49:49PM +0200, David Marchand wrote:
> On Wed, Mar 20, 2024 at 10:25 PM Tyler Retzlaff
> <roretzla@linux.microsoft.com> wrote:
> >
> > provide toolchain abstraction for __builtin_ffs{,l,ll} gcc built-in
> > intrinsics.
> >
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> 
> Please add a unit test and an entry in the release notes.
> 
> > ---
> >  lib/eal/include/rte_bitops.h | 34 ++++++++++++++++++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
> > index 449565e..e157a45 100644
> > --- a/lib/eal/include/rte_bitops.h
> > +++ b/lib/eal/include/rte_bitops.h
> > @@ -405,6 +405,28 @@
> >         return (unsigned int)__popcnt64(v);
> >  }
> >
> > +static inline unsigned int
> > +rte_ffs32(uint32_t v)
> > +{
> > +       unsigned long rv;
> > +
> > +       if (0 == _BitScanForward(&rv, v))
> 
> _BitScanForward(&rv, v) == 0 please.
> 
> 
> -- 
> David Marchand
> 

Thanks David. I'll make the change suggested and add some unit tests.
--
Andre Muezerie

[PATCH v2 0/3] provide rte_ffs32, rte_ffs64 and __rte_x86_movdiri

[Andre Muezerie]

MSVC does not support inline assembly so use movdiri intrinsic and
provide abstracted rte_ffs{32,64} inline functions instead of directly
using GCC built-ins.

v2:
 * Moved constants to the right side of the comparison
 * Added tests for rte_ffs32 and rte_ffs64 functions

Andre Muezerie (1):
  app/test: add test for rte_ffs32 and rte_ffs64 functions.

Tyler Retzlaff (2):
  eal: provide movdiri for MSVC
  eal: add rte ffs32 and rte ffs64 inline functions

 app/test/test_bitops.c       | 38 ++++++++++++++++++++++++++++++++++++
 lib/eal/include/rte_bitops.h | 34 ++++++++++++++++++++++++++++++++
 lib/eal/x86/include/rte_io.h |  4 ++++
 3 files changed, 76 insertions(+)

--
2.47.0.vfs.0.3

[PATCH v2 1/3] eal: provide movdiri for MSVC

[Andre Muezerie]

From: Tyler Retzlaff <roretzla@linux.microsoft.com>

MSVC does not support inline assembly. Instead it provides compiler
intrinsics. Provide conditional compile for MSVC for movdiri using the
_directstoreu_u32 intrinsic.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/x86/include/rte_io.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/eal/x86/include/rte_io.h b/lib/eal/x86/include/rte_io.h
index f92a2e5295..7087e728ce 100644
--- a/lib/eal/x86/include/rte_io.h
+++ b/lib/eal/x86/include/rte_io.h
@@ -22,11 +22,15 @@ extern "C" {
 static __rte_always_inline void
 __rte_x86_movdiri(uint32_t value, volatile void *addr)
 {
+#ifdef RTE_TOOLCHAIN_MSVC
+	_directstoreu_u32((void *)(uintptr_t)addr, value);
+#else
 	asm volatile(
 		/* MOVDIRI */
 		".byte 0x0f, 0x38, 0xf9, 0x02"
 		:
 		: "a" (value), "d" (addr));
+#endif
 }
 
 __rte_experimental
-- 
2.47.0.vfs.0.3

[PATCH v2 2/3] eal: add rte ffs32 and rte ffs64 inline functions

[Andre Muezerie]

From: Tyler Retzlaff <roretzla@linux.microsoft.com>

Provide toolchain abstraction for __builtin_ffs{,l,ll} gcc built-in
intrinsics.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/rte_bitops.h | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
index deb1fd43f2..14074850ec 100644
--- a/lib/eal/include/rte_bitops.h
+++ b/lib/eal/include/rte_bitops.h
@@ -958,6 +958,28 @@ rte_popcount64(uint64_t v)
 	return (unsigned int)__popcnt64(v);
 }
 
+static inline unsigned int
+rte_ffs32(uint32_t v)
+{
+	unsigned long rv;
+
+	if (_BitScanForward(&rv, v) == 0)
+		return 0;
+
+	return (unsigned int)rv + 1;
+}
+
+static inline unsigned int
+rte_ffs64(uint64_t v)
+{
+	unsigned long rv;
+
+	if (_BitScanForward64(&rv, v) == 0)
+		return 0;
+
+	return (unsigned int)rv + 1;
+}
+
 #else
 
 /**
@@ -1044,6 +1066,18 @@ rte_popcount64(uint64_t v)
 	return (unsigned int)__builtin_popcountll(v);
 }
 
+static inline unsigned int
+rte_ffs32(uint32_t v)
+{
+	return (unsigned int)__builtin_ffs(v);
+}
+
+static inline unsigned int
+rte_ffs64(uint64_t v)
+{
+	return (unsigned int)__builtin_ffsll(v);
+}
+
 #endif
 
 /**
-- 
2.47.0.vfs.0.3

[PATCH v2 3/3] app/test: add test for rte_ffs32 and rte_ffs64 functions.

[Andre Muezerie]

Add tests for new rte_ffs32 and rte_ffs64 functions.

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

diff --git a/app/test/test_bitops.c b/app/test/test_bitops.c
index 78a7df6bb1..dd374d7883 100644
--- a/app/test/test_bitops.c
+++ b/app/test/test_bitops.c
@@ -406,6 +406,42 @@ test_bit_relaxed_test_set_clear(void)
 	return TEST_SUCCESS;
 }
 
+static int
+test_bit_scan_forward(void)
+{
+	unsigned int bit_nr;
+
+	TEST_ASSERT((bit_nr = rte_ffs32(0)) == 0,
+		"rte_ffs32 returned unexpected %d", bit_nr);
+
+	for (int i = 0; i < 32; ++i) {
+		uint32_t n = RTE_BIT32(i);
+
+		TEST_ASSERT((bit_nr = rte_ffs32(n)) == (unsigned int)(i+1),
+			"rte_ffs32 returned unexpected %d", bit_nr);
+	}
+
+	return TEST_SUCCESS;
+}
+
+static int
+test_bit_scan_forward64(void)
+{
+	unsigned int bit_nr;
+
+	TEST_ASSERT((bit_nr = rte_ffs64(0)) == 0,
+		"rte_ffs64 returned unexpected %d", bit_nr);
+
+	for (int i = 0; i < 64; ++i) {
+		uint64_t n = RTE_BIT64(i);
+
+		TEST_ASSERT((bit_nr = rte_ffs64(n)) == (unsigned int)(i+1),
+			"rte_ffs64 returned unexpected %d", bit_nr);
+	}
+
+	return TEST_SUCCESS;
+}
+
 static struct unit_test_suite test_suite = {
 	.suite_name = "Bitops test suite",
 	.unit_test_cases = {
@@ -428,6 +464,8 @@ static struct unit_test_suite test_suite = {
 		TEST_CASE(test_bit_relaxed_set),
 		TEST_CASE(test_bit_relaxed_clear),
 		TEST_CASE(test_bit_relaxed_test_set_clear),
+		TEST_CASE(test_bit_scan_forward),
+		TEST_CASE(test_bit_scan_forward64),
 		TEST_CASES_END()
 	}
 };
-- 
2.47.0.vfs.0.3

Re: [PATCH v2 0/3] provide rte_ffs32, rte_ffs64 and __rte_x86_movdiri

[David Marchand]

On Thu, Dec 5, 2024 at 9:36 PM Andre Muezerie
<andremue@linux.microsoft.com> wrote:
>
> MSVC does not support inline assembly so use movdiri intrinsic and
> provide abstracted rte_ffs{32,64} inline functions instead of directly
> using GCC built-ins.
>
> v2:
>  * Moved constants to the right side of the comparison
>  * Added tests for rte_ffs32 and rte_ffs64 functions
>
> Andre Muezerie (1):
>   app/test: add test for rte_ffs32 and rte_ffs64 functions.
>
> Tyler Retzlaff (2):
>   eal: provide movdiri for MSVC
>   eal: add rte ffs32 and rte ffs64 inline functions
>
>  app/test/test_bitops.c       | 38 ++++++++++++++++++++++++++++++++++++
>  lib/eal/include/rte_bitops.h | 34 ++++++++++++++++++++++++++++++++
>  lib/eal/x86/include/rte_io.h |  4 ++++
>  3 files changed, 76 insertions(+)
>

I see nothing wrong with adding those wrappers to ease MSVC support.
Just, those two ffs helpers should be marked experimental.

And the unit tests for counting/searching bits had been separated in a
dedicated app/test/test_bitcount.c.


-- 
David Marchand

[PATCH v3 0/3] provide rte_ffs32, rte_ffs64 and __rte_x86_movdiri

[Andre Muezerie]

MSVC does not support inline assembly so use movdiri intrinsic and
provide abstracted rte_ffs{32,64} inline functions instead of directly
using GCC built-ins.

v3:
 * Added __rte_experimental to the new functions.
 * Added comments to the new functions.
 * Moved tests to test_bitcount.c

v2:
 * Moved constants to the right side of the comparison
 * Added tests for rte_ffs32 and rte_ffs64 functions

Andre Muezerie (1):
  app/test: add test for rte_ffs32 and rte_ffs64 functions.

Tyler Retzlaff (2):
  eal: provide movdiri for MSVC
  eal: add rte ffs32 and rte ffs64 inline functions

 app/test/test_bitcount.c     | 38 ++++++++++++++++++
 lib/eal/include/rte_bitops.h | 74 ++++++++++++++++++++++++++++++++++++
 lib/eal/x86/include/rte_io.h |  4 ++
 3 files changed, 116 insertions(+)

--
2.47.2.vfs.0.1

[PATCH v3 1/3] eal: provide movdiri for MSVC

[Andre Muezerie]

From: Tyler Retzlaff <roretzla@linux.microsoft.com>

MSVC does not support inline assembly. Instead it provides compiler
intrinsics. Provide conditional compile for MSVC for movdiri using the
_directstoreu_u32 intrinsic.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/x86/include/rte_io.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/eal/x86/include/rte_io.h b/lib/eal/x86/include/rte_io.h
index f92a2e5295..7087e728ce 100644
--- a/lib/eal/x86/include/rte_io.h
+++ b/lib/eal/x86/include/rte_io.h
@@ -22,11 +22,15 @@ extern "C" {
 static __rte_always_inline void
 __rte_x86_movdiri(uint32_t value, volatile void *addr)
 {
+#ifdef RTE_TOOLCHAIN_MSVC
+	_directstoreu_u32((void *)(uintptr_t)addr, value);
+#else
 	asm volatile(
 		/* MOVDIRI */
 		".byte 0x0f, 0x38, 0xf9, 0x02"
 		:
 		: "a" (value), "d" (addr));
+#endif
 }
 
 __rte_experimental
-- 
2.47.2.vfs.0.1

[PATCH v3 2/3] eal: add rte ffs32 and rte ffs64 inline functions

[Andre Muezerie]

From: Tyler Retzlaff <roretzla@linux.microsoft.com>

Provide toolchain abstraction for __builtin_ffs{,l,ll} gcc built-in
intrinsics.

Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
---
 lib/eal/include/rte_bitops.h | 74 ++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h
index deb1fd43f2..0862fd4008 100644
--- a/lib/eal/include/rte_bitops.h
+++ b/lib/eal/include/rte_bitops.h
@@ -958,6 +958,48 @@ rte_popcount64(uint64_t v)
 	return (unsigned int)__popcnt64(v);
 }
 
+/**
+ * Search v from least significant bit (LSB) to the most
+ * significant bit (MSB) for a set bit (1).
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   Bit index + 1 if a set bit is found, zero otherwise.
+ */
+__rte_experimental
+static inline unsigned int
+rte_ffs32(uint32_t v)
+{
+	unsigned long rv;
+
+	if (_BitScanForward(&rv, v) == 0)
+		return 0;
+
+	return (unsigned int)rv + 1;
+}
+
+/**
+ * Search v from least significant bit (LSB) to the most
+ * significant bit (MSB) for a set bit (1).
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   Bit index + 1 if a set bit is found, zero otherwise.
+ */
+__rte_experimental
+static inline unsigned int
+rte_ffs64(uint64_t v)
+{
+	unsigned long rv;
+
+	if (_BitScanForward64(&rv, v) == 0)
+		return 0;
+
+	return (unsigned int)rv + 1;
+}
+
 #else
 
 /**
@@ -1044,6 +1086,38 @@ rte_popcount64(uint64_t v)
 	return (unsigned int)__builtin_popcountll(v);
 }
 
+/**
+ * Search v from least significant bit (LSB) to the most
+ * significant bit (MSB) for a set bit (1).
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   Bit index + 1 if a set bit is found, zero otherwise.
+ */
+__rte_experimental
+static inline unsigned int
+rte_ffs32(uint32_t v)
+{
+	return (unsigned int)__builtin_ffs(v);
+}
+
+/**
+ * Search v from least significant bit (LSB) to the most
+ * significant bit (MSB) for a set bit (1).
+ *
+ * @param v
+ *   The value.
+ * @return
+ *   Bit index + 1 if a set bit is found, zero otherwise.
+ */
+__rte_experimental
+static inline unsigned int
+rte_ffs64(uint64_t v)
+{
+	return (unsigned int)__builtin_ffsll(v);
+}
+
 #endif
 
 /**
-- 
2.47.2.vfs.0.1

[PATCH v3 3/3] app/test: add test for rte_ffs32 and rte_ffs64 functions.

[Andre Muezerie]

Add tests for new rte_ffs32 and rte_ffs64 functions.

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

diff --git a/app/test/test_bitcount.c b/app/test/test_bitcount.c
index 83c68feb7b..8eb4a460f2 100644
--- a/app/test/test_bitcount.c
+++ b/app/test/test_bitcount.c
@@ -12,6 +12,42 @@
 
 RTE_LOG_REGISTER(bitcount_logtype_test, test.bitcount, INFO);
 
+static int
+test_bit_scan_forward(void)
+{
+	unsigned int bit_nr;
+
+	TEST_ASSERT((bit_nr = rte_ffs32(0)) == 0,
+		"rte_ffs32 returned unexpected %d", bit_nr);
+
+	for (int i = 0; i < 32; ++i) {
+		uint32_t n = RTE_BIT32(i);
+
+		TEST_ASSERT((bit_nr = rte_ffs32(n)) == (unsigned int)(i+1),
+			"rte_ffs32 returned unexpected %d", bit_nr);
+	}
+
+	return TEST_SUCCESS;
+}
+
+static int
+test_bit_scan_forward64(void)
+{
+	unsigned int bit_nr;
+
+	TEST_ASSERT((bit_nr = rte_ffs64(0)) == 0,
+		"rte_ffs64 returned unexpected %d", bit_nr);
+
+	for (int i = 0; i < 64; ++i) {
+		uint64_t n = RTE_BIT64(i);
+
+		TEST_ASSERT((bit_nr = rte_ffs64(n)) == (unsigned int)(i+1),
+			"rte_ffs64 returned unexpected %d", bit_nr);
+	}
+
+	return TEST_SUCCESS;
+}
+
 static int
 test_clz32(void)
 {
@@ -117,6 +153,8 @@ static struct unit_test_suite bitcount_test_suite = {
 	.setup = NULL,
 	.teardown = NULL,
 	.unit_test_cases = {
+		TEST_CASE(test_bit_scan_forward),
+		TEST_CASE(test_bit_scan_forward64),
 		TEST_CASE(test_clz32),
 		TEST_CASE(test_clz64),
 		TEST_CASE(test_ctz32),
-- 
2.47.2.vfs.0.1

Re: [PATCH v2 0/3] provide rte_ffs32, rte_ffs64 and __rte_x86_movdiri

[Andre Muezerie]

On Fri, Jan 24, 2025 at 03:53:58PM +0100, David Marchand wrote:
> On Thu, Dec 5, 2024 at 9:36 PM Andre Muezerie
> <andremue@linux.microsoft.com> wrote:
> >
> > MSVC does not support inline assembly so use movdiri intrinsic and
> > provide abstracted rte_ffs{32,64} inline functions instead of directly
> > using GCC built-ins.
> >
> > v2:
> >  * Moved constants to the right side of the comparison
> >  * Added tests for rte_ffs32 and rte_ffs64 functions
> >
> > Andre Muezerie (1):
> >   app/test: add test for rte_ffs32 and rte_ffs64 functions.
> >
> > Tyler Retzlaff (2):
> >   eal: provide movdiri for MSVC
> >   eal: add rte ffs32 and rte ffs64 inline functions
> >
> >  app/test/test_bitops.c       | 38 ++++++++++++++++++++++++++++++++++++
> >  lib/eal/include/rte_bitops.h | 34 ++++++++++++++++++++++++++++++++
> >  lib/eal/x86/include/rte_io.h |  4 ++++
> >  3 files changed, 76 insertions(+)
> >
> 
> I see nothing wrong with adding those wrappers to ease MSVC support.
> Just, those two ffs helpers should be marked experimental.
> 
> And the unit tests for counting/searching bits had been separated in a
> dedicated app/test/test_bitcount.c.
> 
> 
> -- 
> David Marchand

Thanks for the review David. I sent a v3 series addressing these issues.