Message-ID: <20250212135200.5a1f2dca@pumpkin>
Date: Wed, 12 Feb 2025 13:52:00 +0000
From: David Laight <david.laight.linux@...il.com>
To: I Hsin Cheng <richard120310@...il.com>
Cc: yury.norov@...il.com, anshuman.khandual@....com, arnd@...db.de,
 linux-kernel@...r.kernel.org, jserv@...s.ncku.edu.tw,
 skhan@...uxfoundation.org
Subject: Re: [PATCH 2/2] uapi: Refactor __GENMASK_ULL() for speed-up

On Wed, 12 Feb 2025 20:39:09 +0800
I Hsin Cheng <richard120310@...il.com> wrote:

> On Tue, Feb 11, 2025 at 10:30:45PM +0000, David Laight wrote:
> > On Wed, 12 Feb 2025 00:24:12 +0800
> > I Hsin Cheng <richard120310@...il.com> wrote:
> >   
> > > The calculation of "((~_ULL(0)) - (_ULL(1) << (l)) + 1)" is to generate
> > > a bitmask with "l" trailing zeroes, which is equivalent to
> > > "(~_ULL(0) << (l))".  
> > 
> > Yes, and if you look through the commit history you'll see it was changed
> > to avoid a compiler warning about the shift losing significant bits.
> > So you are just reverting that change and the compiler warnings will
> > reappear.
> > 
> > For non-constants I suspect that (2ul << hi) - (1ul << lo) is the
> > best answer.
> > But the compiler (clang with some options?) will still complain
> > for constants when trying to set the high bit.
> > 
> > That version also doesn't need BITS_PER_[U]LONG.
> > While that is valid for C, the _ULL() are there for the assembler
> > (when they are no-ops) so there is no way asm copies can be right
> > for both GENMASK() ans GENMASK_ULL().
> > 
> > 	David  
> 
> Hi David,
> 
> Thanks for the review!
> 
> > Yes, and if you look through the commit history you'll see it was changed
> > to avoid a compiler warning about the shift losing significant bits.  
> 
> I've browse through the commits of include/linux/bits.h , where
> GENMASK() was placed under. Though I didn't find specific commit of it,
> would you be so kind to paste the link of the commit?
> 
> I assume you're talking about warnings like the following?
> warning: left shift count >= width of type [-Wshift-count-overflow]

No, there is one about clang 'objecting' to ~0u << count because
significant bits get discarded.
The 'strange' expression was used to avoid that - even though it is
mathematically equivalent.

> 
> If this is the case then it happens for the current version as well, I
> mean from the perspective of operations, "(~_ULL(0) << (l))" and
> "(_ULL(1) << (1))" are basically the same, they all shift a constant
> left with an amount of "l". When "l" is large enough the compiler will
> complain about losing msb.
> 
> > While that is valid for C, the _ULL() are there for the assembler
> > (when they are no-ops) so there is no way asm copies can be right
> > for both GENMASK() ans GENMASK_ULL().  
> 
> I don't understand this part sorry, if asm copies can't be right for
> "~_ULL(0) << (l)" , how can it be right for "(_ULL(1) << (l))" ?
> At least from my pespective these 2 ops only differs at the value of
> constant. Please let me know about it, I'm not familiar with assembler
> and would love to know more about it. Thanks.

It is the >> that go wrong, since they rely on the number of bits in
the integer type being used.
For asm both ~_UL(0) and ~_ULL(0) are just ~0, but the >> uses a
different constant.
I'm not even sure that the _ULL() should even be in a uapi header.

I proposed using:
	((1u << (hi)) - 1) * 2) + 1 - (((1u << (lo)) - 1)
not that long ago.
That avoid any bits being shifted off the top and I'm sure it
that gcc optimised add the +/- away.
It also has the advantage that the type of the result only depends
of the type of the 1u (so can be 1ul, 1ull etc).

	David

> 
> Best regards,
> I Hsin Cheng
> 

Powered by blists - more mailing lists