[PATCH 1/2] bitset: discontinue the use of GCC builtin

[PATCH 1/2] bitset: discontinue the use of GCC builtin

[Mattias Rönnblom]

Replace the use of __builtin_ffsll() with rte_bsf64() to be MSVC
compatible.

Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
Suggested-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/include/rte_bitset.h | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/lib/eal/include/rte_bitset.h b/lib/eal/include/rte_bitset.h
index 74c643a72a..27b7a2e34d 100644
--- a/lib/eal/include/rte_bitset.h
+++ b/lib/eal/include/rte_bitset.h
@@ -601,7 +601,6 @@ __rte_bitset_find_nowrap(const uint64_t *bitset, size_t __rte_unused size, size_
 
 	while (word_idx <= __RTE_BITSET_WORD_IDX(end_bit - 1)) {
 		uint64_t word;
-		int word_ffs;
 
 		word = bitset[word_idx];
 		if (find_clear)
@@ -609,16 +608,14 @@ __rte_bitset_find_nowrap(const uint64_t *bitset, size_t __rte_unused size, size_
 
 		word >>= offset;
 
-		word_ffs = __builtin_ffsll(word);
-
-		if (word_ffs != 0) {
-			ssize_t ffs = start_bit + word_ffs - 1;
+		if (word != 0) {
+			size_t ffs = start_bit + rte_bsf64(word);
 
 			/*
 			 * Check if set bit were among the last,
 			 * unused bits, in the last word.
 			 */
-			if (unlikely(ffs >= (ssize_t)end_bit))
+			if (unlikely(ffs >= end_bit))
 				return -1;
 
 			return ffs;
-- 
2.43.0

[PATCH 2/2] devtools: forbid the use of ffs compiler builtins

[Mattias Rönnblom]

Extend checkpatches.sh to detect the use of __builtin_(ffs|ffsll).

These intrinsics are not available in MSVC, and there are perfectly
serviceable alternatives in <rte_bitops.h>.

Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
Suggested-by: David Marchand <david.marchand@redhat.com>
---
 devtools/checkpatches.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
index c23792025a..411c40d275 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -179,7 +179,7 @@ check_forbidden_additions() { # <patch>
 
 	# forbid use of non abstracted bit count operations
 	awk -v FOLDERS="lib drivers app examples" \
-		-v EXPRESSIONS='\\<__builtin_(clz|clzll|ctz|ctzll|popcount|popcountll)\\>' \
+		-v EXPRESSIONS='\\<__builtin_(clz|clzll|ctz|ctzll|popcount|popcountll|ffs|ffsll)\\>' \
 		-v RET_ON_FAIL=1 \
 		-v MESSAGE='Using __builtin helpers for bit count operations' \
 		-f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
-- 
2.43.0

Re: [PATCH 2/2] devtools: forbid the use of ffs compiler builtins

[Stephen Hemminger]

On Wed, 16 Oct 2024 15:54:11 +0200
Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:

> Extend checkpatches.sh to detect the use of __builtin_(ffs|ffsll).
> 
> These intrinsics are not available in MSVC, and there are perfectly
> serviceable alternatives in <rte_bitops.h>.
> 
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Suggested-by: David Marchand <david.marchand@redhat.com>

Shouldn't this apply to all _builtin_ functions.

There are a lot of drivers still doing this.

Re: [PATCH 2/2] devtools: forbid the use of ffs compiler builtins

[David Marchand]

On Wed, Oct 16, 2024 at 5:04 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Wed, 16 Oct 2024 15:54:11 +0200
> Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
>
> > Extend checkpatches.sh to detect the use of __builtin_(ffs|ffsll).
> >
> > These intrinsics are not available in MSVC, and there are perfectly
> > serviceable alternatives in <rte_bitops.h>.
> >
> > Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > Suggested-by: David Marchand <david.marchand@redhat.com>
>
> Shouldn't this apply to all _builtin_ functions.
>
> There are a lot of drivers still doing this.

- The updated check here is about use of __builtin_XXX instead of DPDK
bit count ops.
So when you refer to all builtin functions, there may be other
builtins related to bit count we are missing, but at least the current
patch is better than before.


- On the larger topic of refusing *any* __builtin_, some use of them
are "legit" under compiler checks.
So the check would have to skip headers where wrappers are provided to
the rest of DPDK.
This seems possible, but a larger and probably more tricky change for now.

Are you ok if we go with this patch as is, in a first step?


-- 
David Marchand

Re: [PATCH 2/2] devtools: forbid the use of ffs compiler builtins

[Stephen Hemminger]

On Wed, 16 Oct 2024 17:32:27 +0200
David Marchand <david.marchand@redhat.com> wrote:

> On Wed, Oct 16, 2024 at 5:04 PM Stephen Hemminger
> <stephen@networkplumber.org> wrote:
> >
> > On Wed, 16 Oct 2024 15:54:11 +0200
> > Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
> >  
> > > Extend checkpatches.sh to detect the use of __builtin_(ffs|ffsll).
> > >
> > > These intrinsics are not available in MSVC, and there are perfectly
> > > serviceable alternatives in <rte_bitops.h>.
> > >
> > > Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > > Suggested-by: David Marchand <david.marchand@redhat.com>  
> >
> > Shouldn't this apply to all _builtin_ functions.
> >
> > There are a lot of drivers still doing this.  
> 
> - The updated check here is about use of __builtin_XXX instead of DPDK
> bit count ops.
> So when you refer to all builtin functions, there may be other
> builtins related to bit count we are missing, but at least the current
> patch is better than before.
> 
> 
> - On the larger topic of refusing *any* __builtin_, some use of them
> are "legit" under compiler checks.
> So the check would have to skip headers where wrappers are provided to
> the rest of DPDK.
> This seems possible, but a larger and probably more tricky change for now.
> 
> Are you ok if we go with this patch as is, in a first step?
> 
> 

Examples of direct use of builtin
	app/dumpcap/main.c:     log2 = sizeof(size) * 8 - __builtin_clzl(size - 1);
	app/test-dma-perf/benchmark.c:                  __builtin_ia32_clflush(data + offset);
	drivers/bus/fslmc/qbman/include/compat.h:#define likely(x)      __builtin_expect(!!(x), 1)
	drivers/bus/fslmc/qbman/include/compat.h:#define unlikely(x)    __builtin_expect(!!(x), 0)

	drivers/common/nfp/nfp_platform.h:#define __bf_shf(x) (__builtin_ffsll(x) - 1)
	drivers/crypto/ccp/ccp_dev.c:   first_zero = __builtin_ffsl(~word);
	app/test/test_memcpy_perf.c:    if (__builtin_constant_p(n))  

	drivers/net/bonding/rte_eth_bond_pmd.c:         if (__builtin_popcountl(link_speeds) != 2) {


Think that direct use of __builitin_expect should be flagged.
And the lots of __builtin_in mlx5 should also be flagged.

		

	 

Re: [PATCH 2/2] devtools: forbid the use of ffs compiler builtins

[David Marchand]

On Wed, Oct 16, 2024 at 5:39 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Wed, 16 Oct 2024 17:32:27 +0200
> David Marchand <david.marchand@redhat.com> wrote:
>
> > On Wed, Oct 16, 2024 at 5:04 PM Stephen Hemminger
> > <stephen@networkplumber.org> wrote:
> > >
> > > On Wed, 16 Oct 2024 15:54:11 +0200
> > > Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
> > >
> > > > Extend checkpatches.sh to detect the use of __builtin_(ffs|ffsll).
> > > >
> > > > These intrinsics are not available in MSVC, and there are perfectly
> > > > serviceable alternatives in <rte_bitops.h>.
> > > >
> > > > Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> > > > Suggested-by: David Marchand <david.marchand@redhat.com>
> > >
> > > Shouldn't this apply to all _builtin_ functions.
> > >
> > > There are a lot of drivers still doing this.
> >
> > - The updated check here is about use of __builtin_XXX instead of DPDK
> > bit count ops.
> > So when you refer to all builtin functions, there may be other
> > builtins related to bit count we are missing, but at least the current
> > patch is better than before.
> >
> >
> > - On the larger topic of refusing *any* __builtin_, some use of them
> > are "legit" under compiler checks.
> > So the check would have to skip headers where wrappers are provided to
> > the rest of DPDK.
> > This seems possible, but a larger and probably more tricky change for now.
> >
> > Are you ok if we go with this patch as is, in a first step?
> >
> >
>
> Examples of direct use of builtin
>         app/dumpcap/main.c:     log2 = sizeof(size) * 8 - __builtin_clzl(size - 1);
>         app/test-dma-perf/benchmark.c:                  __builtin_ia32_clflush(data + offset);
>         drivers/bus/fslmc/qbman/include/compat.h:#define likely(x)      __builtin_expect(!!(x), 1)
>         drivers/bus/fslmc/qbman/include/compat.h:#define unlikely(x)    __builtin_expect(!!(x), 0)
>
>         drivers/common/nfp/nfp_platform.h:#define __bf_shf(x) (__builtin_ffsll(x) - 1)
>         drivers/crypto/ccp/ccp_dev.c:   first_zero = __builtin_ffsl(~word);
>         app/test/test_memcpy_perf.c:    if (__builtin_constant_p(n))
>
>         drivers/net/bonding/rte_eth_bond_pmd.c:         if (__builtin_popcountl(link_speeds) != 2) {
>
>
> Think that direct use of __builitin_expect should be flagged.
> And the lots of __builtin_in mlx5 should also be flagged.

Yes, we should fix those, but that's beyond the bitops check.
I'll go ahead with current series for now.


-- 
David Marchand

Re: [PATCH 2/2] devtools: forbid the use of ffs compiler builtins

[Mattias Rönnblom]

On 2024-10-16 17:04, Stephen Hemminger wrote:
> On Wed, 16 Oct 2024 15:54:11 +0200
> Mattias Rönnblom <mattias.ronnblom@ericsson.com> wrote:
> 
>> Extend checkpatches.sh to detect the use of __builtin_(ffs|ffsll).
>>
>> These intrinsics are not available in MSVC, and there are perfectly
>> serviceable alternatives in <rte_bitops.h>.
>>
>> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
>> Suggested-by: David Marchand <david.marchand@redhat.com>
> 
> Shouldn't this apply to all _builtin_ functions.
> 

Aren̈́'t GCC builtins pretty much standard? So any driver not targeting 
Windows should be fine, although it would be better to use a DPDK wrapper.

> There are a lot of drivers still doing this.

I would suggest we fix this when someone has taken the time to 
improve/modernize/extend <rte_bitops.h> further (e.g., with _Generic 
versions of all bit fiddling and count functions and "all" __builtins 
are covered).

I guess other APIs also may need to be extended (for non-bitops builtins).

Re: [PATCH 1/2] bitset: discontinue the use of GCC builtin

[David Marchand]

On Wed, Oct 16, 2024 at 4:03 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> Replace the use of __builtin_ffsll() with rte_bsf64() to be MSVC
> compatible.
>
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>
> Suggested-by: David Marchand <david.marchand@redhat.com>

Thank you, lgtm.


-- 
David Marchand

Re: [PATCH 1/2] bitset: discontinue the use of GCC builtin

[David Marchand]

On Wed, Oct 16, 2024 at 4:03 PM Mattias Rönnblom
<mattias.ronnblom@ericsson.com> wrote:
>
> Replace the use of __builtin_ffsll() with rte_bsf64() to be MSVC
> compatible.
>
> Suggested-by: David Marchand <david.marchand@redhat.com>
> Signed-off-by: Mattias Rönnblom <mattias.ronnblom@ericsson.com>

Series applied, thanks.


-- 
David Marchand