Received-SPF: pass (google.com: domain of linux-kernel+bounces-160887-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <56414029e179d219442bde9b8eae81fa3e3ceec4.camel@xry111.site>
Subject: Re: [PATCH] LoongArch: Provide __lshrti3, __ashrti3, and __ashrti3
From: Xi Ruoyao <xry111@xry111.site>
To: Huacai Chen <chenhuacai@kernel.org>
Cc: WANG Xuerui <kernel@xen0n.name>, loongarch@lists.linux.dev, 
	linux-kernel@vger.kernel.org, Tiezhu Yang <yangtiezhu@loongson.cn>
Date: Sat, 27 Apr 2024 12:13:25 +0800
In-Reply-To: <dfdc9823a0b89c8582587fa75448bba5c3a7e15e.camel@xry111.site>
References: <20240426121442.882029-1-xry111@xry111.site>
	 <CAAhV-H74cQ4XdDez5PipCxUZTpfS=CA6azL5qob=jGGebobD6g@mail.gmail.com>
	 <dfdc9823a0b89c8582587fa75448bba5c3a7e15e.camel@xry111.site>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.52.1 
Precedence: bulk
MIME-Version: 1.0

On Sat, 2024-04-27 at 12:00 +0800, Xi Ruoyao wrote:
> On Sat, 2024-04-27 at 10:50 +0800, Huacai Chen wrote:
> > Hi, Ruoyao,
> >=20
> > I don't think #ifdef CONFIG_ARCH_SUPPORTS_INT128 is needed here.
> > S390/ARM64/RISCV all built it unconditionally.
>=20
> The problem here is RISCV and ARM64 are using an incorrect prototype for
> these functions in asm-prototypes.h:
>=20
> long long __lshrti3(long long a, int b);=20
> long long __ashrti3(long long a, int b);=20
> long long __ashlti3(long long a, int b);=20
>=20
> where "long long" is not 128-bit.=C2=A0 Despite this seems working for RI=
SC-V
> and ARM64 I really dislike it.
>=20
> S390 seems assuming CONFIG_ARCH_SUPPORTS_INT128 is always true, but I
> don't think we can assume it too (at least it'll likely to be false for
> LA32, so doing so will cause trouble when we add LA32 support).
>=20
> So if we don't want to check CONFIG_ARCH_SUPPORTS_INT128 and still use a
> correct prototype, we'll do:
>=20
> diff --git a/arch/loongarch/include/asm/asm-prototypes.h b/arch/loongarch=
/include/asm/asm-prototypes.h
> index 51f224bcfc65..0a57db01116d 100644
> --- a/arch/loongarch/include/asm/asm-prototypes.h
> +++ b/arch/loongarch/include/asm/asm-prototypes.h
> @@ -7,8 +7,6 @@
> =C2=A0#include <asm/ftrace.h>
> =C2=A0#include <asm-generic/asm-prototypes.h>
> =C2=A0
> -#ifdef CONFIG_ARCH_SUPPORTS_INT128
> -__int128_t __ashlti3(__int128_t a, int b);
> -__int128_t __ashrti3(__int128_t a, int b);
> -__int128_t __lshrti3(__int128_t a, int b);
> -#endif
> +struct { u64 lo, hi; } __ashlti3(u64 lo, u64 hi, int b);
> +struct { u64 lo, hi; } __ashrti3(u64 lo, u64 hi, int b);
> +struct { u64 lo, hi; } __lshrti3(u64 lo, u64 hi, int b);

Whoops.  This is still incorrect for LA32.  On LA32 an "int128" (if it
ever exists) should be passed as a pointer, but this is passing it in 4
GPRs.  So if we want to keep the prototype correct we need to either use
"struct { u64 lo, hi; }" in the parameter list too, or guard it with
#ifdef CONFIG_64BIT.

So to me checking CONFIG_ARCH_SUPPORTS_INT128 is just easier.

If you insists on not checking CONFIG_ARCH_SUPPORTS_INT128 I'll just use
an incorrect prototype like RISC-V but put a comment here, like:

/* The prototypes are incorrect but this file is only used by
   modpost which does not care.  */
long long __ashlti3(long long a, int b);
long long __ashrti3(long long a, int b);
long long __lshrti3(long long a, int b);

How do you think?

--=20
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University