Re: [PATCH v2 06/13] xen/bitops: Implement ffs() in common logic

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 2024-05-31 10:48, Andrew Cooper wrote:
> On 31/05/2024 9:34 am, Andrew Cooper wrote:
> > On 31/05/2024 7:56 am, Nicola Vetrini wrote:
> > > On 2024-05-31 03:14, Stefano Stabellini wrote:
> > > > On Fri, 24 May 2024, Andrew Cooper wrote:
> > > > > Perform constant-folding unconditionally, rather than having it
> > > > > implemented
> > > > > inconsistency between architectures.
> > > > >
> > > > > Confirm the expected behaviour with compile time and boot time 
> > > > > tests.
> > > > > For non-constant inputs, use arch_ffs() if provided but fall back 
> > > > > to
> > > > > generic_ffsl() if not.  In particular, RISC-V doesn't have a 
> > > > > builtin
> > > > > that
> > > > > works in all configurations.
> > > > >
> > > > > For x86, rename ffs() to arch_ffs() and adjust the prototype.
> > > > >
> > > > > For PPC, __builtin_ctz() is 1/3 of the size of size of the 
> > > > > transform to
> > > > > generic_fls().  Drop the definition entirely.  ARM too benefits in
> > > > > the general
> > > > > case by using __builtin_ctz(), but less dramatically because it 
> > > > > using
> > > > > optimised asm().
> > > > >
> > > > > Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> > > > This patch made me realize that we should add __builtin_ctz,
> > > > __builtin_constant_p and always_inline to
> > > > docs/misra/C-language-toolchain.rst as they don't seem to be 
> > > > currently
> > > > documented and they are not part of the C standard
> > > >
> > > > Patch welcome :-)
> > > I can send a patch for the builtins.
> > That's very kind of you.
> >
> > In total by the end of this series, we've got __builtin_constant_p() 
> > (definitely used elsewhere already), and 
> > __builtin_{ffs,ctz,clz}{,l}() 
> > (3x primitives, 2x input types).
> >
> > If we're going for a list of the primitive operations, lets add
> > __builtin_popcnt{,l}() too right away, because if it weren't for 4.19
> > code freeze, I'd have cleaned up the hweight() helpers too.
> Oh, and it's worth noting that __builtin_{ctz,clz}{,l}() have explicit
> UB if given an input of 0.  (Sadly, even on architectures where the
> underlying instruction emitted is safe with a 0 input. [0])
>
> This is why every patch in the series using them checks for nonzero 
> input.
> UBSAN (with an adequate compiler) will instrument this, and Xen has
> __ubsan_handle_invalid_builtin() to diagnose these.
>
> ~Andrew
>
> [0] It turns out that Clang has a 2-argument form of the builtin with
> the second being the "value forwarded" in case the first is 0.  I've 
> not
> investigated whether GCC has the same.
Hmm, maybe then it's best if builtins are listed in a separate section 
in that file, for ease of browsing. Xen also uses (conditionally) 
__builtin_mem*, __builtin_str* and others, so if all nonstandard 
intrinsics should be listed (as opposed to the ones in some way relevant 
for MISRA violations, which was the original scope of the document), 
then a subset of the attached list would be needed. There are a handful 
only used in ppc, and since the document only covers x86 and arm, those 
should be ignored for the time being.
Anyway, I'll send an RFC next week to decide the best route.

--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)

Attachment: builtins.txt
Description: Text document