Re: [PATCH 1/1] bitfield.h: Ensure FIELD_PREP_CONST() is constant

[David Laight]

On Sat, 11 Apr 2026 00:24:28 -0400
Yury Norov <ynorov@xxxxxxxxxx> wrote:

> On Fri, Apr 10, 2026 at 07:45:38PM +0100, David Laight wrote:
> > On Fri, 10 Apr 2026 12:55:25 -0400
> > Yury Norov <ynorov@xxxxxxxxxx> wrote:  
>  
>  ...
> 
> > > > Note that when 'val' is a variable 'val << constant' is likely
> > > > to execute faster than 'val * (1 << constant)'.
> > > > So the normal FIELD_PREP() is best left alone.    
> > > 
> > > Do you have any numbers? I'd prefer to have the codebase consistent
> > > when possible.  
> > 
> > I think the multiply instruction will have a higher latency than the shift.
> > So you are talking about a very small number of clocks if the expression
> > is in the critical register dependency path.
> > However FIELD_GET() would need to use a divide - and that would be a lot
> > worse.
> >
> > Having written that, ISTR that 'mask' is required to be a constant.
> > So the compiler may use a shift anyway - if the divide is unsigned.
> > But for non-constant mask you definitely want a 'shift right'.  
> 
> Non-constant masks are handled with __field_get(), which doesn't use
> __bf_shf().
> 
> > While you might think that it only makes sense to use unsigned values,
> > I've found one piece of code (IIRC in the x86 fault handler) that
> > passes a signed value to FIELD_GET() and needs the result sign extended.
> > So, unless that is changed, FIELD_GET() must use an explicit right shift.
> > (Of course, right shift of negative values is probably UB...)  
> 
> FIELD_GET() is quite fine with the change:
> 
>  #define __FIELD_GET(mask, reg, pfx)                                    \
>          ({                                                              \
>                  __BF_FIELD_CHECK_MASK(mask, 0U, pfx);                   \
>  -               (typeof(mask))(((reg) & (mask)) >> __bf_shf(mask));     \
>  +               (typeof(mask))(((reg) & (mask)) / __bf_low_bit(mask));  \
>          })
>  
> void my_test(void)
> {
>         f3 0f 1e fa             endbr64
>         48 83 ec 08             sub    $0x8,%rsp
>         volatile int i = -1;
> 
>         pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i));
>         48 c7 c7 13 e3 51 82    mov    $0xffffffff8251e313,%rdi
>         volatile int i = -1;
>         c7 44 24 04 ff ff ff    movl   $0xffffffff,0x4(%rsp)
>         ff 
>         pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i));
>         8b 74 24 04             mov    0x4(%rsp),%esi
> 
> }
>         48 83 c4 08             add    $0x8,%rsp
>         pr_err("%lx\n", FIELD_GET(GENMASK(10,5), i));
>         81 e6 e0 07 00 00       and    $0x7e0,%esi
>         48 c1 ee 05             shr    $0x5,%rsi
>         e9 32 aa b9 ff          jmp    <_printk>

There is a subtle difference between (https://www.godbolt.org/z/KM7MesPWM):

int a(int x)
{
    return x >> __bf_shf(0xf0u);
}

int b(int x)
{
    return x / __bf_low_bit(0xf0);
}

int c(int x)
{
    return x / __bf_low_bit(0xf0u);
}

a:
        movl    %edi, %eax
        sarl    $4, %eax
        ret
b:
        testl   %edi, %edi
        leal    15(%rdi), %eax
        cmovns  %edi, %eax
        sarl    $4, %eax
        ret
c:
        movl    %edi, %eax
        shrl    $4, %eax
        ret

A while ago I did a compile-test for negative values and found one
place that requires the sign-replicating right shift.

So you'd need that check and to fixup the caller.

	David

> 
> Thanks,
> Yury