[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <19ed618c-5be9-4658-a2a3-031f4eded19a@app.fastmail.com>
Date: Wed, 24 Jul 2024 14:37:34 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Anshuman Khandual" <anshuman.khandual@....com>,
linux-kernel@...r.kernel.org
Cc: "Andrew Morton" <akpm@...ux-foundation.org>,
"Yury Norov" <yury.norov@...il.com>,
"Rasmus Villemoes" <linux@...musvillemoes.dk>,
Linux-Arch <linux-arch@...r.kernel.org>
Subject: Re: [PATCH 1/2] uapi: Define GENMASK_U128
On Wed, Jul 24, 2024, at 13:59, Anshuman Khandual wrote:
> On 7/24/24 16:33, Arnd Bergmann wrote:
>> I would hope we don't need this definition. Not that it
>> hurts at all, but __BITS_PER_LONG_LONG was already kind
>> of pointless since we don't run on anything else and
>> __BITS_PER_U128 clearly can't have any other sensible
>> definition than a plain 128.
>
> Agreed, although this just followed __BITS_PER_LONG_LONG.
> But sure __BITS_PER_U128 can be plain 128.
>
> So would you like to have #ifndef __BITS_PER_LONG_LONG dropped here
> as well ? But should that be folded or in a separate patch ?
A separate patch is probably better, but you can also
just leave it.
>>> #define __AC(X,Y) (X##Y)
>>> #define _AC(X,Y) __AC(X,Y)
>>> #define _AT(T,X) ((T)(X))
>>> +#define _AC128(X) ((unsigned __int128)(X))
>>
>> I just tried using this syntax and it doesn't seem to do
>> what you expected. gcc silently truncates the constant
>
> But numbers passed into _AC128() are smaller in the range [128..0].
> Hence the truncation might not be problematic in this context ? OR
> could it be ?
>
>> to a 64-bit value here, while clang fails the build.
>
> Should this be disabled for CC_IS_CLANG ?
>
>> See also https://godbolt.org/z/rzEqra7nY
>> https://stackoverflow.com/questions/63328802/unsigned-int128-literal-gcc
>
> So unless the value in there is beyond 64 bits, it should be good ?
> OR am I missing something.
>
>> The __GENMASK_U128() macro however seems to work correctly
>> since you start out with a smaller number and then shift
>> it after the type conversion.
>
> _U128() never receives anything beyond [127..0] range. So then this
> should be good ?
Since you define _U128() right next to _ULL(), I would argue
that it should have the corresponding behavior for any value
that can fit into the type. Since that is currently not
possible with gcc, I would prefer to not define it at all.
However, I think you can just define a _BIT128() macro
that behaves the same way as _BITULL() and define
__GENMASK_U128() based on that. Maybe something like
#define _BIT128(x) ((unsigned __int128)1 << (x))
#define __GENMASK_U128(h, l) (_BIT128((h) + 1)) - (_BIT128(l))
Arnd
Powered by blists - more mailing lists