[v2,2/5] lib/eal: add portable version of __builtin_add_overflow

__builtin_add_overflow is gcc specific. There's a need for a portable
version that can also be used with other compilers.

This patch introduces rte_add_overflow.

Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
---
 lib/eal/include/meson.build |  1 +
 lib/eal/include/rte_math.h  | 46 +++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)
 create mode 100644 lib/eal/include/rte_math.h
  

On Fri, Jan 03, 2025 at 12:39:38PM -0800, Andre Muezerie wrote:
> __builtin_add_overflow is gcc specific. There's a need for a portable
> version that can also be used with other compilers.
> 
> This patch introduces rte_add_overflow.
> 
> Signed-off-by: Andre Muezerie <andremue@linux.microsoft.com>
> ---
>  lib/eal/include/meson.build |  1 +
>  lib/eal/include/rte_math.h  | 46 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+)
>  create mode 100644 lib/eal/include/rte_math.h
> 
> diff --git a/lib/eal/include/meson.build b/lib/eal/include/meson.build
> index d903577caa..041a4105b5 100644
> --- a/lib/eal/include/meson.build
> +++ b/lib/eal/include/meson.build
> @@ -31,6 +31,7 @@ headers += files(
>          'rte_lcore_var.h',
>          'rte_lock_annotations.h',
>          'rte_malloc.h',
> +        'rte_math.h',
>          'rte_mcslock.h',
>          'rte_memory.h',
>          'rte_memzone.h',
> diff --git a/lib/eal/include/rte_math.h b/lib/eal/include/rte_math.h
> new file mode 100644
> index 0000000000..2f4581f81b
> --- /dev/null
> +++ b/lib/eal/include/rte_math.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2025 Microsoft Corporation
> + */
> +
> +#ifndef _RTE_MATH_H_
> +#define _RTE_MATH_H_
> +
> +/**
> + * @file
> + *
> + * Math function definitions for DPDK.
> + */
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +/*
> + * Function that allows performing simple arithmetic operations together with
> + * checking whether the operation overflowed.
> + * Example of usage:
> + *     uint8_t overflow;
> + *     uint16_t a, b, result;
> + *     a = 1;
> + *     b = 2;
> + *     overflow = rte_add_overflow(a, b, &result);
> + */
> +#ifdef RTE_TOOLCHAIN_MSVC
> +#define rte_add_overflow(a, b, res) _Generic((a), \
> +	uint8_t : _addcarry_u8, \
> +	uint16_t : _addcarry_u16, \
> +	uint32_t : _addcarry_u32, \
> +	uint64_t : _addcarry_u64)(0, a, b, res)
> +#else
> +#define rte_add_overflow(a, b, res) _Generic((a), \
> +	uint8_t : __builtin_add_overflow, \
> +	uint16_t : __builtin_add_overflow, \
> +	uint32_t : __builtin_add_overflow, \
> +	uint64_t : __builtin_add_overflow)(a, b, res)
> +#endif

For the gcc version, can you just simplify to the one-line below?

#define rte_add_overflow __builtin_add_overflow

/Bruce
  

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 6 January 2025 12.07
> 
> On Fri, Jan 03, 2025 at 12:39:38PM -0800, Andre Muezerie wrote:
> > __builtin_add_overflow is gcc specific. There's a need for a portable
> > version that can also be used with other compilers.
> >
> > This patch introduces rte_add_overflow.
> >
> > +/*
> > + * Function that allows performing simple arithmetic operations
> together with
> > + * checking whether the operation overflowed.
> > + * Example of usage:
> > + *     uint8_t overflow;
> > + *     uint16_t a, b, result;
> > + *     a = 1;
> > + *     b = 2;
> > + *     overflow = rte_add_overflow(a, b, &result);
> > + */
> > +#ifdef RTE_TOOLCHAIN_MSVC
> > +#define rte_add_overflow(a, b, res) _Generic((a), \
> > +	uint8_t : _addcarry_u8, \
> > +	uint16_t : _addcarry_u16, \
> > +	uint32_t : _addcarry_u32, \
> > +	uint64_t : _addcarry_u64)(0, a, b, res)
> > +#else
> > +#define rte_add_overflow(a, b, res) _Generic((a), \
> > +	uint8_t : __builtin_add_overflow, \
> > +	uint16_t : __builtin_add_overflow, \
> > +	uint32_t : __builtin_add_overflow, \
> > +	uint64_t : __builtin_add_overflow)(a, b, res)
> > +#endif
> 
> For the gcc version, can you just simplify to the one-line below?
> 
> #define rte_add_overflow __builtin_add_overflow

Yes, but then GCC compilation would not fail if "a" has some other type than the four types explicitly supported.
I prefer keeping the method used this v2 patch.
  

On Mon, Jan 06, 2025 at 12:21:39PM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Monday, 6 January 2025 12.07
> > 
> > On Fri, Jan 03, 2025 at 12:39:38PM -0800, Andre Muezerie wrote:
> > > __builtin_add_overflow is gcc specific. There's a need for a portable
> > > version that can also be used with other compilers.
> > >
> > > This patch introduces rte_add_overflow.
> > >
> > > +/*
> > > + * Function that allows performing simple arithmetic operations
> > together with
> > > + * checking whether the operation overflowed.
> > > + * Example of usage:
> > > + *     uint8_t overflow;
> > > + *     uint16_t a, b, result;
> > > + *     a = 1;
> > > + *     b = 2;
> > > + *     overflow = rte_add_overflow(a, b, &result);
> > > + */
> > > +#ifdef RTE_TOOLCHAIN_MSVC
> > > +#define rte_add_overflow(a, b, res) _Generic((a), \
> > > +	uint8_t : _addcarry_u8, \
> > > +	uint16_t : _addcarry_u16, \
> > > +	uint32_t : _addcarry_u32, \
> > > +	uint64_t : _addcarry_u64)(0, a, b, res)
> > > +#else
> > > +#define rte_add_overflow(a, b, res) _Generic((a), \
> > > +	uint8_t : __builtin_add_overflow, \
> > > +	uint16_t : __builtin_add_overflow, \
> > > +	uint32_t : __builtin_add_overflow, \
> > > +	uint64_t : __builtin_add_overflow)(a, b, res)
> > > +#endif
> > 
> > For the gcc version, can you just simplify to the one-line below?
> > 
> > #define rte_add_overflow __builtin_add_overflow
> 
> Yes, but then GCC compilation would not fail if "a" has some other type than the four types explicitly supported.
> I prefer keeping the method used this v2 patch.
>
Is that really a problem? Should our DPDK macro not support all the types
that the GCC builtin supports?

/Bruce
  

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 6 January 2025 12.34
> 
> On Mon, Jan 06, 2025 at 12:21:39PM +0100, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Monday, 6 January 2025 12.07
> > >
> > > On Fri, Jan 03, 2025 at 12:39:38PM -0800, Andre Muezerie wrote:
> > > > __builtin_add_overflow is gcc specific. There's a need for a
> portable
> > > > version that can also be used with other compilers.
> > > >
> > > > This patch introduces rte_add_overflow.
> > > >
> > > > +/*
> > > > + * Function that allows performing simple arithmetic operations
> > > together with
> > > > + * checking whether the operation overflowed.
> > > > + * Example of usage:
> > > > + *     uint8_t overflow;
> > > > + *     uint16_t a, b, result;
> > > > + *     a = 1;
> > > > + *     b = 2;
> > > > + *     overflow = rte_add_overflow(a, b, &result);
> > > > + */
> > > > +#ifdef RTE_TOOLCHAIN_MSVC
> > > > +#define rte_add_overflow(a, b, res) _Generic((a), \
> > > > +	uint8_t : _addcarry_u8, \
> > > > +	uint16_t : _addcarry_u16, \
> > > > +	uint32_t : _addcarry_u32, \
> > > > +	uint64_t : _addcarry_u64)(0, a, b, res)
> > > > +#else
> > > > +#define rte_add_overflow(a, b, res) _Generic((a), \
> > > > +	uint8_t : __builtin_add_overflow, \
> > > > +	uint16_t : __builtin_add_overflow, \
> > > > +	uint32_t : __builtin_add_overflow, \
> > > > +	uint64_t : __builtin_add_overflow)(a, b, res)
> > > > +#endif
> > >
> > > For the gcc version, can you just simplify to the one-line below?
> > >
> > > #define rte_add_overflow __builtin_add_overflow
> >
> > Yes, but then GCC compilation would not fail if "a" has some other
> type than the four types explicitly supported.
> > I prefer keeping the method used this v2 patch.
> >
> Is that really a problem? Should our DPDK macro not support all the
> types
> that the GCC builtin supports?

The DPDK macro should support all the types that both MSVC and GCC supports.
Using _Generic() for GCC is an improvement for the CI to catch MSVC incompatible code when building for GCC.

Only these four unsigned types are supported by the x86_64 intrinsics.
I don't think we need support for more types; but if the need should arise, it can be added later.
  

On Mon, Jan 06, 2025 at 12:58:44PM +0100, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Monday, 6 January 2025 12.34
> > 
> > On Mon, Jan 06, 2025 at 12:21:39PM +0100, Morten Brørup wrote:
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > Sent: Monday, 6 January 2025 12.07
> > > >
> > > > On Fri, Jan 03, 2025 at 12:39:38PM -0800, Andre Muezerie wrote:
> > > > > __builtin_add_overflow is gcc specific. There's a need for a
> > portable
> > > > > version that can also be used with other compilers.
> > > > >
> > > > > This patch introduces rte_add_overflow.
> > > > >
> > > > > +/*
> > > > > + * Function that allows performing simple arithmetic operations
> > > > together with
> > > > > + * checking whether the operation overflowed.
> > > > > + * Example of usage:
> > > > > + *     uint8_t overflow;
> > > > > + *     uint16_t a, b, result;
> > > > > + *     a = 1;
> > > > > + *     b = 2;
> > > > > + *     overflow = rte_add_overflow(a, b, &result);
> > > > > + */
> > > > > +#ifdef RTE_TOOLCHAIN_MSVC
> > > > > +#define rte_add_overflow(a, b, res) _Generic((a), \
> > > > > +	uint8_t : _addcarry_u8, \
> > > > > +	uint16_t : _addcarry_u16, \
> > > > > +	uint32_t : _addcarry_u32, \
> > > > > +	uint64_t : _addcarry_u64)(0, a, b, res)
> > > > > +#else
> > > > > +#define rte_add_overflow(a, b, res) _Generic((a), \
> > > > > +	uint8_t : __builtin_add_overflow, \
> > > > > +	uint16_t : __builtin_add_overflow, \
> > > > > +	uint32_t : __builtin_add_overflow, \
> > > > > +	uint64_t : __builtin_add_overflow)(a, b, res)
> > > > > +#endif
> > > >
> > > > For the gcc version, can you just simplify to the one-line below?
> > > >
> > > > #define rte_add_overflow __builtin_add_overflow
> > >
> > > Yes, but then GCC compilation would not fail if "a" has some other
> > type than the four types explicitly supported.
> > > I prefer keeping the method used this v2 patch.
> > >
> > Is that really a problem? Should our DPDK macro not support all the
> > types
> > that the GCC builtin supports?
> 
> The DPDK macro should support all the types that both MSVC and GCC supports.
> Using _Generic() for GCC is an improvement for the CI to catch MSVC incompatible code when building for GCC.
> 
> Only these four unsigned types are supported by the x86_64 intrinsics.
> I don't think we need support for more types; but if the need should arise, it can be added later.

Great. Let me know if any further action is needed on this patch.