Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()

[Yury Norov]

On Mon, Apr 20, 2026 at 01:19:40PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 17, 2026 at 01:36:12PM -0400, Yury Norov wrote:
> > The bitfields are designed in assumption that fields contain unsigned
> > integer values, thus extracting the values from the field implies
> > zero-extending.
> > 
> > Some drivers need to sign-extend their fields, and currently do it like:
> > 
> > 	dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> > 	dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
> > 
> > It's error-prone because it relies on user to provide the correct
> > index of the most significant bit and proper 32 vs 64 function flavor.
> > 
> > Thus, introduce a FIELD_GET_SIGNED() macro, which is the more
> > convenient and compiles (on x86_64) to just a couple instructions:
> > shl and sar.
> > 
> > Signed-off-by: Yury Norov <ynorov@xxxxxxxxxx>
> > ---
> >  include/linux/bitfield.h | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> > 
> > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > index 54aeeef1f0ec..35ef63972810 100644
> > --- a/include/linux/bitfield.h
> > +++ b/include/linux/bitfield.h
> > @@ -178,6 +178,22 @@
> >  		__FIELD_GET(_mask, _reg, "FIELD_GET: ");		\
> >  	})
> >  
> > +/**
> > + * FIELD_GET_SIGNED() - extract a signed bitfield element
> > + * @mask: shifted mask defining the field's length and position
> > + * @reg:  value of entire bitfield
> > + *
> > + * Returns the sign-extended field specified by @_mask from the
> > + * bitfield passed in as @_reg by masking and shifting it down.
> > + */
> > +#define FIELD_GET_SIGNED(mask, reg)					\
> > +	({								\
> > +		__BF_FIELD_CHECK(mask, reg, 0U, "FIELD_GET_SIGNED: ");	\
> > +		 ((__signed_scalar_typeof(mask))((long long)(reg) <<	\
> > +		 __builtin_clzll(mask) >> (__builtin_clzll(mask) +	\
> > +						__builtin_ctzll(mask))));\
> > +	})
> 
> IIRC clz is count-leading-zeros and ctz is count-trailing-zeros. Most of
> the other FIELD things use __bf_shf() which is defined in terms of ffs -
> 1 (which is another way of writing ctz).
> 
> So how about you start by redefining __bf_shf() in ctz, and then add
> another helper for the clz and write the thing something like:
> 
> 	((long long)(reg) << __bf_clz(mask)) >> (__bf_clz(mask) + __bf_shf(mask));

So...

I like the shorter form, but whatever we add in the bitfield.h - we'll
have to support it.

For example, __bf_shf() wasn't intended to be used outsize of the
header, thus double underscored. But there's over 100 external users
now. And to make it worse, it's broken for GCC 14 and earlier:

https://lore.kernel.org/all/20260409-field-prep-fix-v1-1-f0e9ae64f63c@xxxxxxxxxx/

So needs to get fixed.

The bitfield.h has two __bf macros: __bf_shf() and __bf_cast_unsigned().
They are thin wrappers, but after all do something with the corresponding
builtins output. The __bf_cls() would be a pure renaming. I'm OK with
that, but some people don't:

https://lore.kernel.org/all/20260303182845.250bb2de@xxxxxxxxxx/

That's why I didn't make FIELD_GET_SIGNED() implementation looking nicer.
If you strongly prefer the shorter version, I can do that in v2.
 
> Also, since the order of the shifts is rather important, I think it
> makes sense to add this extra pair of (), even when not strictly needed,
> just to make it easier to read.

Sure, will do.