<snip>
>
> On 28/07/20 10:24 +0200, Morten Brørup wrote:
> > + Ray and Neil as ABI Policy maintainers.
> >
> > > From: Honnappa Nagarahalli [mailto:honnappa.nagaraha...@arm.com]
> > > Sent: Tuesday, July 28, 2020 4:19 AM
> > >
> > > <snip>
> > > > >
> > > > > > Subject: [dpdk-dev] [PATCH v10 01/10] eal: introduce macro for
> > > bit
> > > > > definition
> > > > > >
> > > > > > There are several drivers which duplicate bit generation macro.
> > > > > > Introduce a generic bit macros so that such drivers avoid
> > > redefining
> > > > > same in
> > > > > > multiple drivers.
> > > > > >
> > > > > > Signed-off-by: Parav Pandit <pa...@mellanox.com>
> > > > > > Acked-by: Matan Azrad <ma...@mellanox.com>
> > > > > > Acked-by: Morten Brørup <m...@smartsharesystems.com>
> > > > > > ---
> > > > > > Changelog:
> > > > > > v4->v5:
> > > > > > - Addressed comments from Morten Brørup
> > > > > > - Renamed newly added macro to RTE_BIT64
> > > > > > - Added doxygen comment section for the macro
> > > > > > v1->v2:
> > > > > > - Addressed comments from Thomas and Gaten.
> > > > > > - Avoided new file, added macro to rte_bitops.h
> > > > > > ---
> > > > > > lib/librte_eal/include/rte_bitops.h | 8 ++++++++
> > > > > > 1 file changed, 8 insertions(+)
> > > > > >
> > > > > > diff --git a/lib/librte_eal/include/rte_bitops.h
> > > > > > b/lib/librte_eal/include/rte_bitops.h
> > > > > > index 740927f3b..ca46a110f 100644
> > > > > > --- a/lib/librte_eal/include/rte_bitops.h
> > > > > > +++ b/lib/librte_eal/include/rte_bitops.h
> > > > > > @@ -17,6 +17,14 @@
> > > > > > #include <rte_debug.h>
> > > > > > #include <rte_compat.h>
> > > > > >
> > > > > > +/**
> > > > > > + * Get the uint64_t value for a specified bit set.
> > > > > > + *
> > > > > > + * @param nr
> > > > > > + * The bit number in range of 0 to 63.
> > > > > > + */
> > > > > > +#define RTE_BIT64(nr) (UINT64_C(1) << (nr))
> > > > > In general, the macros have been avoided in this file. Suggest
> > > > > changing this to an inline function.
> > > >
> > > > That has been discussed already, and rejected for good reasons:
> > > >
> http://inbox.dpdk.org/dev/AM0PR05MB4866823B0170B90F679A2765D1640@
> > > > AM0PR05MB4866.eurprd05.prod.outlook.com/
> > > Thank you for the link.
> > > In this patch series, I see the macro being used in enum
> > > initialization
> > > (7/10 in v11) as well as in functions (8/10 in v11). Does it make
> > > sense to introduce use inline functions and use the inline functions
> > > for 8/10?
> > > If we do this, we should document in rte_bitops.h that inline
> > > functions should be used wherever possible.
> >
> > I would agree, but only in theory. I disagree in reality, and argue that
> > there
> should only be macros for this. Here is why:
> >
> > rte_byteorder.h has both RTE_BEnn() macros and rte_cpu_to_be_nn()
> functions, for doing the same thing at compile time or at run time. There are
> no compile time warnings if the wrong one is being used, so I am certain that
> we can find code that uses the macro where the function should be used, or
> vice versa.
Agree, there is not a suitable way to enforce the use of one over the other
(other than code review).
When the APIs in rte_bitops.h were introduced, there was a discussion around
using the macros. I was for using macros as it would have kept the code as well
as number of APIs smaller. However, there was a decision made not to use macros
and instead provide inline functions. It was nothing to do with performance.
So, I am just saying that we need to follow the same principles at least for
this file.
> >
>
> Hi,
>
> It is not clear to me, reading this thread, what is the motivation to enforce
> use of inline functions? Is it perf, compiler type checking, or usage checks?
>
> Macros are checked at compile time when possible, though it can be
> improved upon. But I agree with Morten, proposing two forms ensures devs
> will sometimes use the wrong one, and we would need a practical way to
> check usages.
>
> > Which opens another, higher level, question: Would it be possible to add a
> compile time check macro in rte_common.h for these and similar?
> >
>
> Can you clarify your idea? Is is something similar to:
>
> #define _BIT64(n) (UINT64_C(1) << (n))
> static inline uint64_t
> bit64(uint64_t n)
> {
> assert(n < 64);
> return (UINT64_C(1) << n);
> }
> /* Integer Constant Expression? */
> #define ICE_P(x) (sizeof(int) == sizeof(*(1 ? ((void*)((x) * 0l)) :
> (int*)1)))
> #define BIT64(n) (ICE_P(n) ? _BIT64(n) : bit64(n))
>
> I don't think so, but this is as close as automatic compile-time check and
> automatic use of proper macro vs. function I know of, did you have something
> else in mind?
>
> In this kind of code:
>
> #include <stdio.h>
> #include <stdint.h>
> #include <inttypes.h>
> #include <assert.h>
>
> enum vals {
> ZERO = 0,
> ONE = BIT64(1),
> TWO = BIT64(2),
> THREE = BIT64(3),
> };
>
> int main(void)
> {
> uint64_t x = ONE;
>
> x = BIT64(0);
> x = BIT64(1);
> x = BIT64(60);
> x = BIT64(64);
> x = BIT64(x);
>
> printf("x: 0x%" PRIx64 "\n", x);
>
> return 0;
> }
>
> The enum is defined using the macro, x = BIT64(64); triggers the following
> warning with GCC:
>
> constant_bitop.c:6:32: warning: left shift count >= width of type [-Wshift-
> count-overflow]
> 6 | #define _BIT64(n) (UINT64_C(1) << (n))
>
> and x = BIT64(x); triggers the assert() at runtime.
>
> > Furthermore: For the RTE_BITnn() operations in this patch set, I expect the
> compiler to generate perfectly efficient code using the macro for run time
> use.
> I.e. there would be no performance advantage by also implementing the
> macros as functions for run time use.
> >
>
> Regards,
> --
> Gaëtan
