From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 8FBB542394; Tue, 10 Jan 2023 19:38:01 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3386440E25; Tue, 10 Jan 2023 19:38:01 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id 42EE14021E for ; Tue, 10 Jan 2023 19:38:00 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 48A2620B92AE; Tue, 10 Jan 2023 10:37:59 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 48A2620B92AE DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1673375879; bh=MMsXqQGsqFfsi3VAF7ZQXD0Kvg18CCwD/+EOOr5dNXI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=shRXHjSFX/FS3HvMMgpqfqqP4fIpQvoVE1dhSN8Ceyw3xxsGlVO9HAgUAOXs30I/N kiB1IaaPIS7omsqL6bCKCOxQbvnHPmvjRfTpTetrvN0vP4UfhDRXhKVqNuMV2R9ifb BxF8b/6DAFFbNuP2qYJgczQy63W7gRl6uo+8f6HU= Date: Tue, 10 Jan 2023 10:37:59 -0800 From: Tyler Retzlaff To: Ferruh Yigit Cc: dev@dpdk.org, thomas@monjalon.net, mb@smartsharesystems.com, bruce.richardson@intel.com, Tyler Retzlaff Subject: Re: [PATCH v4 2/2] eal: provide leading and trailing zero bit count abstraction Message-ID: <20230110183759.GB21476@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1669246997-30592-1-git-send-email-roretzla@linux.microsoft.com> <1673285784-14394-1-git-send-email-roretzla@linux.microsoft.com> <1673285784-14394-3-git-send-email-roretzla@linux.microsoft.com> <7d1d6509-83e0-ce20-ac26-b72f4c0ba02d@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7d1d6509-83e0-ce20-ac26-b72f4c0ba02d@amd.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Tue, Jan 10, 2023 at 01:55:59PM +0000, Ferruh Yigit wrote: > On 1/9/2023 5:36 PM, Tyler Retzlaff wrote: > > From: Tyler Retzlaff > > > > Provide an abstraction for leading and trailing zero bit counting > > functions to hide compiler specific intrinsics and builtins. > > > > Include basic unit test of following functions added. > > > > rte_clz32 > > rte_clz64 > > rte_ctz32 > > rte_ctz64 > > > > Signed-off-by: Tyler Retzlaff > > --- > > app/test/meson.build | 2 + > > app/test/test_bitcount.c | 71 ++++++++++++++++++ > > lib/eal/include/rte_bitops.h | 168 +++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 241 insertions(+) > > create mode 100644 app/test/test_bitcount.c > > > > diff --git a/app/test/meson.build b/app/test/meson.build > > index f34d19e..d1277bc 100644 > > --- a/app/test/meson.build > > +++ b/app/test/meson.build > > @@ -13,6 +13,7 @@ test_sources = files( > > 'test_alarm.c', > > 'test_atomic.c', > > 'test_barrier.c', > > + 'test_bitcount.c', > > 'test_bitops.c', > > 'test_bitmap.c', > > 'test_bpf.c', > > @@ -160,6 +161,7 @@ test_deps += ['bus_pci', 'bus_vdev'] > > fast_tests = [ > > ['acl_autotest', true, true], > > ['atomic_autotest', false, true], > > + ['bitcount_autotest', true, true], > > ['bitmap_autotest', true, true], > > ['bpf_autotest', true, true], > > ['bpf_convert_autotest', true, true], > > diff --git a/app/test/test_bitcount.c b/app/test/test_bitcount.c > > new file mode 100644 > > index 0000000..36eb05c > > --- /dev/null > > +++ b/app/test/test_bitcount.c > > @@ -0,0 +1,71 @@ > > +/* SPDX-License-Identifier: BSD-3-Clause > > + * Copyright (C) 2022 Microsoft Corporation > > + */ > > + > > +#include > > + > > +#include > > +#include > > + > > +#include "test.h" > > + > > +RTE_LOG_REGISTER(bitcount_logtype_test, test.bitcount, INFO); > > + > > +static int > > +test_clz32(void) > > +{ > > + uint32_t v = 1; > > + RTE_TEST_ASSERT(rte_clz32(v) == sizeof(v) * CHAR_BIT - 1, > > + "Unexpected count."); > > + > > + return 0; > > +} > > + > > +static int > > +test_clz64(void) > > +{ > > + uint64_t v = 1; > > + RTE_TEST_ASSERT(rte_clz64(v) == sizeof(v) * CHAR_BIT - 1, > > + "Unexpected count."); > > + > > + return 0; > > +} > > + > > +static int > > +test_ctz32(void) > > +{ > > + uint32_t v = 2; > > + RTE_TEST_ASSERT(rte_ctz32(v) == 1, "Unexpected count."); > > + > > + return 0; > > +} > > + > > +static int > > +test_ctz64(void) > > +{ > > + uint64_t v = 2; > > + RTE_TEST_ASSERT(rte_ctz64(v) == 1, "Unexpected count."); > > + > > + return 0; > > +} > > + > > +static struct unit_test_suite bitcount_test_suite = { > > + .suite_name = "bitcount autotest", > > + .setup = NULL, > > + .teardown = NULL, > > + .unit_test_cases = { > > + TEST_CASE(test_clz32), > > + TEST_CASE(test_clz64), > > + TEST_CASE(test_ctz32), > > + TEST_CASE(test_ctz64), > > + TEST_CASES_END() > > + } > > +}; > > + > > +static int > > +test_bitcount(void) > > +{ > > + return unit_test_suite_runner(&bitcount_test_suite); > > +} > > + > > +REGISTER_TEST_COMMAND(bitcount_autotest, test_bitcount); > > diff --git a/lib/eal/include/rte_bitops.h b/lib/eal/include/rte_bitops.h > > index 531479e..387d7aa 100644 > > --- a/lib/eal/include/rte_bitops.h > > +++ b/lib/eal/include/rte_bitops.h > > @@ -1,5 +1,7 @@ > > /* SPDX-License-Identifier: BSD-3-Clause > > * Copyright(c) 2020 Arm Limited > > + * Copyright(c) 2010-2019 Intel Corporation > > + * Copyright(c) 2023 Microsoft Corporation > > */ > > > > #ifndef _RTE_BITOPS_H_ > > @@ -275,6 +277,172 @@ > > return val & mask; > > } > > > > +#ifdef RTE_TOOLCHAIN_MSVC > > + > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this API may change, or be removed, without prior notice > > + * > > + * Get the count of leading 0-bits in v. > > + * > > + * @param v > > + * The value. > > + * @return > > + * The count of leading zero bits. > > + */ > > +__rte_experimental > > +static inline unsigned int > > +rte_clz32(uint32_t v) > > +{ > > + unsigned long rv; > > + > > + (void)_BitScanReverse(&rv, v); > > + > > + return (unsigned int)rv; > > +} > > > OK, this looks wrong to me, > > > '_BitScanReverse' is [1]: > "Search the mask data from most significant bit (MSB) to least > significant bit (LSB) for a set bit (1)." > > As far as I can see index starts from zero and from lsb to msb. > > So _BitScanReverse() returns following index values: > 0x2 => 1 > 0xffffffff => 31 > > If above is correct, above function doesn't return number of leading > zeros, but it should return "31 - (unsigned int)rv". > > Please check godbolt experiment for above: > https://godbolt.org/z/znYn54f57 > > > As far as I can see unit test is correct, so above should fail with > windows compiler, and I assume you run unit test with windows compiler, > so probably I am missing something but please help me understand. the test is wrong, i will fix both the test and the implementation. thank you for the careful review.