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

Re: [PATCH] RISCV/bitops: Use Zbb to provide arch-optimised bitops




On 3/25/25 5:46 PM, Jan Beulich wrote:
On 25.03.2025 17:35, Oleksii Kurochko wrote:
On 3/7/25 1:12 PM, Andrew Cooper wrote:
On 07/03/2025 12:01 pm, Jan Beulich wrote:
On 07.03.2025 12:50, Oleksii Kurochko wrote:
On 3/6/25 9:19 PM, Andrew Cooper wrote:
On 05/03/2025 7:34 am, Jan Beulich wrote:
I was actually hoping to eliminate BITS_PER_LONG at some point, in favor
of using sizeof(long) * BITS_PER_BYTE. (Surely in common code we could
retain a shorthand of that name, if so desired, but I see no reason why
each arch would need to provide all three BITS_PER_{BYTE,INT,LONG}.)
The concern is legibility and clarity.

This:

      ((x) ? 32 - __builtin_clz(x) : 0)

is a clear _expression_ in a way that this:

      ((x) ? (sizeof(int) * BITS_PER_BYTE) - __builtin_clz(x) : 0)

is not.  The problem is the extra binary _expression_, and this:

      ((x) ? BITS_PER_INT - __builtin_clz(x) : 0)

is still clear, because the reader doesn't have to perform a multiply to
just to figure out what's going on.


It is definitely stupid to have each architecture provide their own
BITS_PER_*.  The compiler is in a superior position to provide those
details, and it should be in a common location.

I don't particularly mind how those constants are derived, but one key
thing that BITS_PER_* can do which sizeof() can't is be used in #ifdef/etc.
What about moving them to xen/config.h? (if it isn't the best one place, any suggestion which is better?)

#define BYTES_PER_INT  (1 << INT_BYTEORDER)
#define BITS_PER_INT  (BYTES_PER_INT << 3)

#define BYTES_PER_LONG (1 << LONG_BYTEORDER)
#define BITS_PER_LONG (BYTES_PER_LONG << 3)
#define BITS_PER_BYTE 8
The *_BYTEORDER's are useless indirection.  BITS_PER_* should be defined
straight up, and this will simplify quite a lot of preprocessing.
Could we really drop *_BYTEORDER?

For example, LONG_BYTEORDER for Arm could be either 2 or 3 depends on Arm32 or Arm64 is used.
The can still #define BITS_PER_LONG to 32 or 64 respectively, can't they?
Yes, but then if we want to move it to xen/config.h then it will be needed to:
in Arm's asm/config.h to have something like:
  #ifdef CONFIG_ARM_32
      #define BITS_PER_LONG 32
  #endif

and then in xen/config.h
  #ifndef BITS_PER_LONG
      #define BITS_PER_LONG 64
  #endif

But I wanted to not have #ifndef BITS_PER_LONG in xen/config.h. (or using CONFIG_ARM_32 in xen/config.h)

If it is okay to have this #ifndef in xen/config.h then we can do in that way.


Also, it seems like the follwoing could be moved there too:

#define POINTER_ALIGN  BYTES_PER_LONG
This one is likely fine to move.

#define BITS_PER_LLONG 64
This one is only fine to move imo when converted to

#define BITS_PER_LONG (BYTES_PER_LLONG << 3)
Erm?  That's mixing long and long long types.  Presuming that's an
errant L, then sure.

#define BITS_PER_BYTE 8
Personally I'd rather leave this per-arch. The others can truly be derived;
this one can't be. If we centralize, imo we should also convert the " << 3"
to " * BITS_PER_BYTE".
It is highly unlikely that Xen will ever run on a system where CHAR_BIT
isn't 8.

So I suggest it stays central to reduce complexity.  If an arch ever
needs to change it, the complexity can be added then.
Does it make sense to ifdef that? Or, at least, before defintion of BITS_PER_BYTE something like:
#if CHAR_BIT != 8
#error "CHAR_BIT isn't 8"
#endif
Where would CHAR_BIT come from? Oh, perhaps you mean __CHAR_BIT__? If all
compilers we can build with supply that value, we could indeed centrally
use either

#define BITS_PER_BYTE __CHAR_BIT__

or

#define BITS_PER_BYTE 8
#if BITS_PER_BYTE != __CHAR_BIT__
# error "..."
#endif
Sorry, I meant __CHAR_BIT__.

Thanks.

~ Oleksii

 


Rackspace

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