Re: [PATCH 03/17] x86: Replace open-coded parity calculation with parity8()

Yury Norov <[email protected]> Mon, 24 Feb 2025 17:30:08 -0500

Newsgroups	dev.linux.lists.brcm80211,org.freedesktop.lists.dri-devel,org.infradead.lists.linux-mtd,org.kernel.vger.bpf,org.kernel.vger.linux-input,org.kernel.vger.linux-kernel,org.kernel.vger.linux-media,org.kernel.vger.linux-serial,org.kernel.vger.linux-wireless,org.kernel.vger.netdev
Message-ID	<Z7zy8AOpAo4JQu8W@thinkpad>

On Mon, Feb 24, 2025 at 02:21:13PM -0800, H. Peter Anvin wrote:
> On February 24, 2025 2:17:29 PM PST, Yury Norov <[email protected]> wrote:
> >On Mon, Feb 24, 2025 at 01:55:28PM -0800, H. Peter Anvin wrote:
> >> On 2/24/25 07:24, Uros Bizjak wrote:
> >> > 
> >> > 
> >> > On 23. 02. 25 17:42, Kuan-Wei Chiu wrote:
> >> > > Refactor parity calculations to use the standard parity8() helper. This
> >> > > change eliminates redundant implementations and improves code
> >> > > efficiency.
> >> > 
> >> > The patch improves parity assembly code in bootflag.o from:
> >> > 
> >> >  Â  58:Â Â Â  89 deÂ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  movÂ Â Â  %ebx,%esi
> >> >  Â  5a:Â Â Â  b9 08 00 00 00Â Â Â Â Â Â Â Â Â Â  movÂ Â Â  $0x8,%ecx
> >> >  Â  5f:Â Â Â  31 d2Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  xorÂ Â Â  %edx,%edx
> >> >  Â  61:Â Â Â  89 f0Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  movÂ Â Â  %esi,%eax
> >> >  Â  63:Â Â Â  89 d7Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  movÂ Â Â  %edx,%edi
> >> >  Â  65:Â Â Â  40 d0 eeÂ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  shrÂ Â Â  %sil
> >> >  Â  68:Â Â Â  83 e0 01Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  andÂ Â Â  $0x1,%eax
> >> >  Â  6b:Â Â Â  31 c2Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  xorÂ Â Â  %eax,%edx
> >> >  Â  6d:Â Â Â  83 e9 01Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  subÂ Â Â  $0x1,%ecx
> >> >  Â  70:Â Â Â  75 efÂ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  jneÂ Â Â  61 <sbf_init+0x51>
> >> >  Â  72:Â Â Â  39 c7Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  cmpÂ Â Â  %eax,%edi
> >> >  Â  74:Â Â Â  74 7fÂ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  jeÂ Â Â Â  f5 <sbf_init+0xe5>
> >> >  Â  76:
> >> > 
> >> > to:
> >> > 
> >> >  Â  54:Â Â Â  89 d8Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  movÂ Â Â  %ebx,%eax
> >> >  Â  56:Â Â Â  ba 96 69 00 00Â Â Â Â Â Â Â Â Â Â  movÂ Â Â  $0x6996,%edx
> >> >  Â  5b:Â Â Â  c0 e8 04Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  shrÂ Â Â  $0x4,%al
> >> >  Â  5e:Â Â Â  31 d8Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  xorÂ Â Â  %ebx,%eax
> >> >  Â  60:Â Â Â  83 e0 0fÂ Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  andÂ Â Â  $0xf,%eax
> >> >  Â  63:Â Â Â  0f a3 c2Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  btÂ Â Â Â  %eax,%edx
> >> >  Â  66:Â Â Â  73 64Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  jaeÂ Â Â  cc <sbf_init+0xbc>
> >> >  Â  68:
> >> > 
> >> > which is faster and smaller (-10 bytes) code.
> >> > 
> >> 
> >> Of course, on x86, parity8() and parity16() can be implemented very simply:
> >> 
> >> (Also, the parity functions really ought to return bool, and be flagged
> >> __attribute_const__.)
> >
> >There was a discussion regarding return type when parity8() was added.
> >The integer type was taken over bool with a sort of consideration that
> >bool should be returned as an answer to some question, like parity_odd().
> >
> >To me it's not a big deal. We can switch to boolean and describe in
> >comment what the 'true' means for the parity() function.
> 
> Bool is really the single-bit type, and gives the compiler more information. You could argue that the function really should be called parity_odd*() in general, but that's kind of excessive IMO.

Yes, I could, but I will not. :) I also feel like bool looks more
natural here.