Message-ID: <Z+69UOemf+H/EHvN@visitorckw-System-Product-Name>
Date: Fri, 4 Apr 2025 00:54:40 +0800
From: Kuan-Wei Chiu <visitorckw@...il.com>
To: Yury Norov <yury.norov@...il.com>, "H. Peter Anvin" <hpa@...or.com>
Cc: "H. Peter Anvin" <hpa@...or.com>,
	David Laight <david.laight.linux@...il.com>,
	Andrew Cooper <andrew.cooper3@...rix.com>,
	Laurent.pinchart@...asonboard.com, airlied@...il.com,
	akpm@...ux-foundation.org, alistair@...ple.id.au,
	andrew+netdev@...n.ch, andrzej.hajda@...el.com,
	arend.vanspriel@...adcom.com, awalls@...metrocast.net, bp@...en8.de,
	bpf@...r.kernel.org, brcm80211-dev-list.pdl@...adcom.com,
	brcm80211@...ts.linux.dev, dave.hansen@...ux.intel.com,
	davem@...emloft.net, dmitry.torokhov@...il.com,
	dri-devel@...ts.freedesktop.org, eajames@...ux.ibm.com,
	edumazet@...gle.com, eleanor15x@...il.com,
	gregkh@...uxfoundation.org, hverkuil@...all.nl,
	jernej.skrabec@...il.com, jirislaby@...nel.org, jk@...abs.org,
	joel@....id.au, johannes@...solutions.net, jonas@...boo.se,
	jserv@...s.ncku.edu.tw, kuba@...nel.org, linux-fsi@...ts.ozlabs.org,
	linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-media@...r.kernel.org, linux-mtd@...ts.infradead.org,
	linux-serial@...r.kernel.org, linux-wireless@...r.kernel.org,
	linux@...musvillemoes.dk, louis.peens@...igine.com,
	maarten.lankhorst@...ux.intel.com, mchehab@...nel.org,
	mingo@...hat.com, miquel.raynal@...tlin.com, mripard@...nel.org,
	neil.armstrong@...aro.org, netdev@...r.kernel.org,
	oss-drivers@...igine.com, pabeni@...hat.com,
	parthiban.veerasooran@...rochip.com, rfoss@...nel.org,
	richard@....at, simona@...ll.ch, tglx@...utronix.de,
	tzimmermann@...e.de, vigneshr@...com, x86@...nel.org
Subject: Re: [PATCH v3 00/16] Introduce and use generic parity16/32/64 helper

On Thu, Apr 03, 2025 at 12:14:04PM -0400, Yury Norov wrote:
> On Thu, Apr 03, 2025 at 10:39:03PM +0800, Kuan-Wei Chiu wrote:
> > On Tue, Mar 25, 2025 at 12:43:25PM -0700, H. Peter Anvin wrote:
> > > On 3/23/25 08:16, Kuan-Wei Chiu wrote:
> > > > 
> > > > Interface 3: Multiple Functions
> > > > Description: bool parity_odd8/16/32/64()
> > > > Pros: No need for explicit casting; easy to integrate
> > > >        architecture-specific optimizations; except for parity8(), all
> > > >        functions are one-liners with no significant code duplication
> > > > Cons: More functions may increase maintenance burden
> > > > Opinions: Only I support this approach
> > > > 
> > > 
> > > OK, so I responded to this but I can't find my reply or any of the
> > > followups, so let me go again:
> > > 
> > > I prefer this option, because:
> > > 
> > > a. Virtually all uses of parity is done in contexts where the sizes of the
> > > items for which parity is to be taken are well-defined, but it is *really*
> > > easy for integer promotion to cause a value to be extended to 32 bits
> > > unnecessarily (sign or zero extend, although for parity it doesn't make any
> > > difference -- if the compiler realizes it.)
> > > 
> > > b. It makes it easier to add arch-specific implementations, notably using
> > > __builtin_parity on architectures where that is known to generate good code.
> > > 
> > > c. For architectures where only *some* parity implementations are
> > > fast/practical, the generic fallbacks will either naturally synthesize them
> > > from components via shift-xor, or they can be defined to use a larger
> > > version; the function prototype acts like a cast.
> > > 
> > > d. If there is a reason in the future to add a generic version, it is really
> > > easy to do using the size-specific functions as components; this is
> > > something we do literally all over the place, using a pattern so common that
> > > it, itself, probably should be macroized:
> > > 
> > > #define parity(x) 				\
> > > ({						\
> > > 	typeof(x) __x = (x);			\
> > > 	bool __y;				\
> > > 	switch (sizeof(__x)) {			\
> > > 		case 1:				\
> > > 			__y = parity8(__x);	\
> > > 			break;			\
> > > 		case 2:				\
> > > 			__y = parity16(__x);	\
> > > 			break;			\
> > > 		case 4:				\
> > > 			__y = parity32(__x);	\
> > > 			break;			\
> > > 		case 8:				\
> > > 			__y = parity64(__x);	\
> > > 			break;			\
> > > 		default:			\
> > > 			BUILD_BUG();		\
> > > 			break;			\
> > > 	}					\
> > > 	__y;					\
> > > })
> > >
> > Thank you for your detailed response and for explaining the rationale
> > behind your preference. The points you outlined in (a)–(d) all seem
> > quite reasonable to me.
> > 
> > Yury,
> > do you have any feedback on this?
> > Thank you.
> 
> My feedback to you:
> 
> I asked you to share any numbers about each approach. Asm listings,
> performance tests, bloat-o-meter. But you did nothing or very little
> in that department. You move this series, and it means you should be
> very well aware of alternative solutions, their pros and cons.
>
I'm willing to run micro-benchmarks, but even with performance data, I
lack the domain knowledge to determine which users care about parity
efficiency. No one in Cc has clarified this either.

> Instead, you started a poll to pick the best solution. This is not
> what I expected, and this is not how the best solution can be found.
> 
> To H. Peter and everyone:
> 
> Thank you for sharing your opinion on this fixed parity(). Your
> arguments may or may not be important, depending on what existing
> users actually need. Unfortunately, Kuan-Wei didn't collect
> performance numbers and opinions from those proposed users.
> 
> I already told that, and I will say again: with the lack of any
> evidence that performance and/or code generation is important here,
> the best solution is one that minimizes maintainers' (my!) burden.
> 
> In other words, bool parity(unsigned long long). I'm OK to maintain
> a macro, as well. I understand that more complicated solutions may be
> more effective. I will take them only if they will be well advocated.
> 
Before Peter suggested an arch-specific implementation, I planned to go
with approach #1, as it minimizes maintenance overhead in the absence
of clear user requirements.

Peter,
Have you identified any users who care about parity efficiency?
If not, do we still need to introduce an arch-specific implementation?

Regards,
Kuan-Wei

> I hope this will help us to stop moving this discussion back and forth
> and save our time, guys.
> 
> Thanks,
> Yury

Powered by blists - more mailing lists