From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from xry111.site (xry111.site [89.208.246.23]) by sourceware.org (Postfix) with ESMTPS id AA5843857357 for ; Tue, 15 Aug 2023 15:04:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org AA5843857357 Authentication-Results: sourceware.org; dmarc=pass (p=reject dis=none) header.from=xry111.site Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=xry111.site DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=xry111.site; s=default; t=1692111878; bh=VIqpZUmIzj+47MUXfzKCSLOWSDoiLjXsF+4Pyr8a3NQ=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=UBfOF1xMtPGJdTpQCwy59aGE0isbNT7eRTAbLXr7DPhOi6c5R5EQXH3OOm+dz5ROw mp8A6edrVS9/MpX3T/R1TOTg17XDwuNEDltaUOGjsC6JTYDKDeyZl4OT7xyYhsm6Cm bA6L6Z6UedoE+xDIlYbLLaw4avmDO44/3Hiclzgw= Received: from [IPv6:2409:8a0c:2a4:2170:40dc:a500:81f0:2] (unknown [IPv6:2409:8a0c:2a4:2170:40dc:a500:81f0:2]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature ECDSA (P-384) server-digest SHA384) (Client did not present a certificate) (Authenticated sender: xry111@xry111.site) by xry111.site (Postfix) with ESMTPSA id 531F9659C0; Tue, 15 Aug 2023 11:04:32 -0400 (EDT) Message-ID: <3155807e4eb64937c6c691b228774358fa682ec9.camel@xry111.site> Subject: Re: [PATCH v1 1/2] LoongArch: Fix pcaddi format string From: Xi Ruoyao To: mengqinggang , WANG Xuerui , binutils@sourceware.org Cc: xuchenghua@loongson.cn, chenglulu@loongson.cn, liuzhensong@loongson.cn, maskray@google.com, chenhuacai@kernel.org, yangtiezhu@loongson.cn Date: Tue, 15 Aug 2023 23:03:57 +0800 In-Reply-To: <8721701b-926b-cb86-6959-e1a7bfd9a22c@loongson.cn> References: <20230809013939.3388720-1-mengqinggang@loongson.cn> <20230809013939.3388720-2-mengqinggang@loongson.cn> <548af2c302efcdb23ae3bedd7249db9e618930be.camel@xry111.site> <06b6500e-6e0c-c210-ad8d-afc913f5aa79@xen0n.name> <54726b86-32fe-c19a-f9eb-4789dc68d537@loongson.cn> <54c5188b1acaf5c31da54e920d4b7c155b4bb3ad.camel@xry111.site> <8721701b-926b-cb86-6959-e1a7bfd9a22c@loongson.cn> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.4 MIME-Version: 1.0 X-Spam-Status: No, score=-6.5 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,LIKELY_SPAM_FROM,SPF_HELO_PASS,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, 2023-08-15 at 19:35 +0800, mengqinggang wrote: > The main reason for adding "<<2"=C2=A0 is that without "<<2" one same imm= ediate > for pcaddi and other 4-byts aligned instructions has a different meaning. > The offset of "pcaddi $t0, 4" and "bl 4" are 16 and 4, this difference ma= y > cause confusion for users. Users writing some new code can always adapt, but old code won't change itself miraculously. In software development the stability of interface is much more important than consistency. Most API existing in the world are not consistent, but they just exist as they are. If we must do this, I think we can add an option (maybe named "-mpcaddi- shift=3D0/2") to control the behavior and emit a warning like "warning: use pcaddi and an immediate input w/o -mpcaddi-shift=3D setting is deprecated; use a label or specifying -mpcaddi-shift=3D0/2 explicitly" In 2.42 we can keep -mpcaddi-shift=3D2 (old behavior) as the default, and in 2.43 we can turn the warning into an error (i. e. if you want to code pcaddi with an immediate instead of a label, you must specify -mpcaddi- shift=3D0 or 2). This will make the "old" code fail to assemble immediately, instead of producing a subtle mine blowing up at runtime. For kernel the fix will also be simpler: KBUILD_AFLAGS +=3D $(call cc- option,-Wa$(comma)-mpcaddi-shift=3D2). > =E5=9C=A8 2023/8/15 =E4=B8=8B=E5=8D=886:44, Xi Ruoyao =E5=86=99=E9=81=93: > > On Tue, 2023-08-15 at 18:00 +0800, mengqinggang wrote: > > > Maybe we can just add support for "pcaddi rd, label" this time. > > > And the hard coded immediate in pcaddi can be changed to label. > > > Add "<<2" to format string can be done after several versions. > > It does not fix the issue.=C2=A0 The kernel still needs nasty binutils > > version checks unless it removes the support for building with > > binutils > > < (the first version supports using the label in pcaddi). > >=20 > > I think we should just *never* change the semantics of an assemble > > grammar construct.=C2=A0 I don't think "pcaddi rd, 16" is really better > > than > > "pcaddi rd, 4" (unless we'll add some 2-byte or 1-byte instructions > > like > > RVC, but then we'll need a new instruction and we should not reuse > > the > > pcaddi mnemonic anyway). > >=20 > > If you really need "16" instead of "4" you can add it as a pseudo > > instruction with a new name.=C2=A0 The pseudo instruction can even > > support > > misaligned values like 14 (expanding it to pcaddi rd, 3 and addi rd, > > rd, > > 2).=C2=A0 It can be named "la.pcaddi" or something (following "la.pcrel= " > > etc). > >=20 > > > =E5=9C=A8 2023/8/11 =E4=B8=8A=E5=8D=8811:08, WANG Xuerui =E5=86=99=E9= =81=93: > > > > On 2023/8/9 19:37, Xi Ruoyao wrote: > > > > > On Wed, 2023-08-09 at 09:39 +0800, mengqinggang wrote: > > > > > > Add "<<2" for pcaddi format string > > > > > > --- > > > > > > =C2=A0=C2=A0=C2=A0opcodes/loongarch-opc.c | 2 +- > > > > > > =C2=A0=C2=A0=C2=A01 file changed, 1 insertion(+), 1 deletion(-) > > > > > >=20 > > > > > > diff --git a/opcodes/loongarch-opc.c b/opcodes/loongarch- > > > > > > opc.c > > > > > > index 2f02e33dbec..7d110683e93 100644 > > > > > > --- a/opcodes/loongarch-opc.c > > > > > > +++ b/opcodes/loongarch-opc.c > > > > > > @@ -564,7 +564,7 @@ static struct loongarch_opcode > > > > > > loongarch_imm_opcodes[] =3D > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0 { 0x10000000, > > > > > > 0xfc000000,=C2=A0=C2=A0=C2=A0=C2=A0"addu16i.d",=C2=A0=C2=A0=C2= =A0=C2=A0"r0:5,r5:5,s10:16",=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 > > > > > > =C2=A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A00 > > > > > > }, > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0 { 0x14000000, > > > > > > 0xfe000000,=C2=A0=C2=A0=C2=A0=C2=A0"lu12i.w",=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0"r0:5,s5:20",=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > > > > > > =C2=A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A00 > > > > > > }, > > > > > > =C2=A0=C2=A0=C2=A0=C2=A0 { 0x16000000, > > > > > > 0xfe000000,=C2=A0=C2=A0=C2=A0=C2=A0"lu32i.d",=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0"r0:5,s5:20",=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > > > > > > =C2=A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A00 > > > > > > }, > > > > > > -=C2=A0 { 0x18000000, > > > > > > 0xfe000000,=C2=A0=C2=A0=C2=A0=C2=A0"pcaddi",=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0"r0:5,s5:20",=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > > > > > > =C2=A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A00 > > > > > > }, > > > > > > +=C2=A0 { 0x18000000, > > > > > > 0xfe000000,=C2=A0=C2=A0=C2=A0=C2=A0"pcaddi",=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0"r0:5,s5:20<<2",=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 > > > > > > =C2=A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A00,=C2=A0=C2=A0=C2=A0= =C2=A0=C2=A0=C2=A00 > > > > > > }, > > > > > The Linux kernel already uses things like "pcaddi t0, 4".=C2=A0 T= o > > > > > me this > > > > > change will break them completely, and fixing it on the kernel > > > > > side will > > > > > be difficult (we'll need to create some nasty gas version > > > > > check). > > > > Thanks for the heads-up, I initially was inclined to accept this > > > > patch > > > > but was hindered by real life, then saw this. The asymmetry is > > > > unfortunately real and here to stay, because in this case code > > > > would > > > > be broken without any build-time errors or warnings, and such > > > > breakage > > > > would go unnoticed until the moment the wrong instruction gets > > > > executed. > > > >=20 > > > > > So I don't think we should make such a backward incompatible > > > > > change > > > > > without a very compelling reason.=C2=A0 You may argue that "<<2" > > > > > has a better > > > > > readability, but if we really need the readability we can > > > > > write > > > > >=20 > > > > > pcaddi t0, 4 # << 2 > > > > >=20 > > > > > in the code anyway. > > > > The immediate operand means "delta in # of instructions": > > > >=20 > > > > > pcaddi t0, 4=C2=A0 # insns > > > > Which is *arguably more* intuitive than the new semantics > > > > implied by > > > > "<<2", since IMO it's more natural to think in terms of > > > > instruction > > > > words when we talk about PC-relative tricks with instruction > > > > fetch in > > > > mind. > > > >=20 > > > > IMO a better way forward could be to document this wart in an > > > > upcoming > > > > revision of the LoongArch ISA manual. (It already contains > > > > assembler > > > > syntax tips for insns like alsl.* so another similar addition > > > > should > > > > be appropriate.) It's not sure at this moment whether an > > > > overhaul of > > > > LoongArch assembler is going to happen, so we have little choice > > > > if we > > > > want to keep downstream fuss to a minimum. >=20 --=20 Xi Ruoyao School of Aerospace Science and Technology, Xidian University