[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] [RFC] x86/cpu: rework instruction set selection



On April 26, 2025 12:55:13 PM PDT, Linus Torvalds 
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>On Sat, 26 Apr 2025 at 12:24, Linus Torvalds
><torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>>
>> (And yes, one use in a x86 header file that is pretty questionable
>> too: I think the reason for the cmov is actually i486-only behavior
>> and we could probably unify the 32-bit and 64-bit implementation)
>
>Actually, what we *should* do is to remove that manual use of 'cmov'
>entirely - even if we decide that yes, that undefined zero case is
>actually real.
>
>We should probably change it to use CC_SET(), and the compiler will do
>a much better job - and probably never use cmov anyway.
>
>And yes, that will generate worse code if you have an old compiler
>that doesn't do ASM_FLAG_OUTPUTS, but hey, that's true in general. If
>you want good code, you need a good compiler.
>
>And clang needs to learn the CC_SET() pattern anyway.
>
>So I think that manual cmov pattern for x86-32 should be replaced with
>
>        bool zero;
>
>        asm("bsfl %[in],%[out]"
>            CC_SET(z)
>            : CC_OUT(z) (zero),
>              [out]"=r" (r)
>            : [in] "rm" (x));
>
>        return zero ? 0 : r+1;
>
>instead (that's ffs(), and fls() would need the same thing except with
>bsrl insteadm, of course).
>
>I bet that would actually improve code generation.
>
>And I also bet it doesn't actually matter, of course.
>
>           Linus

It is unfortunate, if understandable, that we ended up using a convention other 
than what ended up becoming standard. (Return the size in bits if the input is 
0.)

This would let us use __builtin_ctz() > tzcnt which I believe is always inline 
on x86, and probably would help several other architectures too.

How much of a pain would it really be to fix this interface?



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.