Re: [PATCH v5 3/8] arm64/runtime-const: Introduce runtime_const_mask_32()

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello,

On 6/30/2026 10:37 AM, sashiko-bot@kernel.org wrote:
> [You don't often get email from sashiko-bot@kernel.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Calling `__fls(val)` before `BUG_ON(!val)` invokes undefined behavior for `val == 0`, potentially allowing the compiler to optimize away the safety check.
> - [Low] The commit message incorrectly references `amd64` instead of `arm64` and misspells `density`.
> --
> 
> commit e9b79c78d336baf757f7578713831df9b1c4c1b9
> Author: K Prateek Nayak <kprateek.nayak@amd.com>
> 
> arm64/runtime-const: Introduce runtime_const_mask_32()
> 
> This patch introduces runtime_const_mask_32 to optimize the mask operation
> in the futex hash computation hot path using the UBFX instruction. It also
> adds __runtime_fixup_mask to patch the 32-bit mask in the asm block once
> the mask is known.
> 
> Link: https://developer.arm.com/documentation/ddi0602/2026-03/Base-Instructions/ [1]
> Link: https://lore.kernel.org/lkml/20260430094730.31624-4-kprateek.nayak@amd.com/ [2]
> 
>> instruction is used for amd64 to improve instruction dinsity and
>> performance.
> 
> [Severity: Low]
> This isn't a bug, but there are a couple of typos here. Should "amd64" be
> "arm64", and "dinsity" be "density"?

Ah! Yes. Sorry about that. I'll wait for a while for more comments and
spin a v5.1 if necessary to fix those typos.

> 
>> diff --git a/arch/arm64/include/asm/runtime-const.h b/arch/arm64/include/asm/runtime-const.h
>> index 838145bc289d2..371c9a4bc2d4b 100644
>> --- a/arch/arm64/include/asm/runtime-const.h
>> +++ b/arch/arm64/include/asm/runtime-const.h
> [ ... ]
>> +static inline void __runtime_fixup_mask(void *where, unsigned long val)
>> +{
>> +     unsigned int width = __fls(val) + 1;
>> +     __le32 *p = where;
>> +     u32 insn;
>> +
>> +     /*
> [ ... ]
>> +      */
>> +     BUG_ON(!val || width > 32 || (GENMASK(width - 1, 0) != val));
> 
> [Severity: Medium]
> Does evaluating __fls(val) before the BUG_ON(!val) check invoke undefined
> behavior if val is 0?
> 
> Looking at __fls(), it uses __builtin_clzl(word) which is undefined when
> the argument is 0:
> 
> include/asm-generic/bitops/builtin-__fls.h:__fls() {
>     return (sizeof(word) * 8) - 1 - __builtin_clzl(word);
> }
> 
> Could the compiler use Value Range Propagation to assume val is non-zero
> due to the __fls() call, and silently optimize away the !val safety guard?

How does that work? "width" is calculated as __fls(val) so why would the
compiler make assumption on the possible values of "val" based on that call
alone?

I suppose you are referring to an issue like
https://gcc.gnu.org/pipermail/gcc/2023-November/242808.html but that is
-O3 and under normal circumstances, the propagation is stopped if the
compiler believes there is possibility of undefined behavior:
https://github.com/gcc-mirror/gcc/blob/7ebc766bdbc8584699acb3f8fd30b2c393923c06/gcc/gimple-range-op.cc#L937-L944

That said, I can fix it in the next version to always check for 0 before
__fls().

-- 
Thanks and Regards,
Prateek