From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.loongson.cn (mail.loongson.cn [114.242.206.163]) by sourceware.org (Postfix) with ESMTP id 8F91E3858404 for ; Tue, 15 Aug 2023 11:35:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8F91E3858404 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=loongson.cn Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=loongson.cn Received: from loongson.cn (unknown [10.20.4.171]) by gateway (Coremail) with SMTP id _____8AxqOj2YttkwrsYAA--.14899S3; Tue, 15 Aug 2023 19:35:18 +0800 (CST) Received: from [10.20.4.171] (unknown [10.20.4.171]) by localhost.localdomain (Coremail) with SMTP id AQAAf8Cx7yP1YttkeR5bAA--.52594S3; Tue, 15 Aug 2023 19:35:17 +0800 (CST) Subject: Re: [PATCH v1 1/2] LoongArch: Fix pcaddi format string To: Xi Ruoyao , WANG Xuerui , binutils@sourceware.org Cc: xuchenghua@loongson.cn, chenglulu@loongson.cn, liuzhensong@loongson.cn, maskray@google.com, chenhuacai@kernel.org, yangtiezhu@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> From: mengqinggang Message-ID: <8721701b-926b-cb86-6959-e1a7bfd9a22c@loongson.cn> Date: Tue, 15 Aug 2023 19:35:17 +0800 User-Agent: Mozilla/5.0 (X11; Linux mips64; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: <54c5188b1acaf5c31da54e920d4b7c155b4bb3ad.camel@xry111.site> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US X-CM-TRANSID:AQAAf8Cx7yP1YttkeR5bAA--.52594S3 X-CM-SenderInfo: 5phqw15lqjwttqj6z05rqj20fqof0/ X-Coremail-Antispam: 1Uk129KBj93XoWxGw15Ar18JF1UuFy8Gw13WrX_yoWrKFyUpF yrJF15JFWkJr1ktr1Dtr1UXry5tr1UAw1UXr1UtF1jyrnxtry2qF4UXr1Y9F1DJr4xCw1j qw1Utr17ZF1UA3gCm3ZEXasCq-sJn29KB7ZKAUJUUUU8529EdanIXcx71UUUUU7KY7ZEXa sCq-sGcSsGvfJ3Ic02F40EFcxC0VAKzVAqx4xG6I80ebIjqfuFe4nvWSU5nxnvy29KBjDU 0xBIdaVrnRJUUU9ab4IE77IF4wAFF20E14v26r1j6r4UM7CY07I20VC2zVCF04k26cxKx2 IYs7xG6rWj6s0DM7CIcVAFz4kK6r1Y6r17M28lY4IEw2IIxxk0rwA2F7IY1VAKz4vEj48v e4kI8wA2z4x0Y4vE2Ix0cI8IcVAFwI0_Jr0_JF4l84ACjcxK6xIIjxv20xvEc7CjxVAFwI 0_Jr0_Gr1l84ACjcxK6I8E87Iv67AKxVW8Jr0_Cr1UM28EF7xvwVC2z280aVCY1x0267AK xVW8Jr0_Cr1UM2AIxVAIcxkEcVAq07x20xvEncxIr21l57IF6xkI12xvs2x26I8E6xACxx 1l5I8CrVACY4xI64kE6c02F40Ex7xfMcIj6xIIjxv20xvE14v26r106r15McIj6I8E87Iv 67AKxVWUJVW8JwAm72CE4IkC6x0Yz7v_Jr0_Gr1lF7xvr2IY64vIr41lc7I2V7IY0VAS07 AlzVAYIcxG8wCF04k20xvY0x0EwIxGrwCFx2IqxVCFs4IE7xkEbVWUJVW8JwCFI7km07C2 67AKxVWUAVWUtwC20s026c02F40E14v26r1j6r18MI8I3I0E7480Y4vE14v26r106r1rMI 8E67AF67kF1VAFwI0_Jw0_GFylIxkGc2Ij64vIr41lIxAIcVC0I7IYx2IY67AKxVWUJVWU CwCI42IY6xIIjxv20xvEc7CjxVAFwI0_Jr0_Gr1lIxAIcVCF04k26cxKx2IYs7xG6r1j6r 1xMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2jsIEc7CjxVAFwI0_Jr0_GrUvcSsG vfC2KfnxnUUI43ZEXa7IU8j-e5UUUUU== X-Spam-Status: No, score=-11.3 required=5.0 tests=BAYES_00,BODY_8BITS,GIT_PATCH_0,KAM_DMARC_STATUS,NICE_REPLY_A,SPF_HELO_NONE,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: The main reason for adding "<<2"  is that without "<<2" one same immediate 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 may cause confusion for users. 在 2023/8/15 下午6:44, Xi Ruoyao 写道: > 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. 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). > > I think we should just *never* change the semantics of an assemble > grammar construct. 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). > > If you really need "16" instead of "4" you can add it as a pseudo > instruction with a new name. The pseudo instruction can even support > misaligned values like 14 (expanding it to pcaddi rd, 3 and addi rd, rd, > 2). It can be named "la.pcaddi" or something (following "la.pcrel" > etc). > >> 在 2023/8/11 上午11:08, WANG Xuerui 写道: >>> 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 >>>>> --- >>>>>   opcodes/loongarch-opc.c | 2 +- >>>>>   1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> 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[] = >>>>>     { 0x10000000, >>>>> 0xfc000000,    "addu16i.d",    "r0:5,r5:5,s10:16",             0,                      0,      0,      0 >>>>> }, >>>>>     { 0x14000000, >>>>> 0xfe000000,    "lu12i.w",      "r0:5,s5:20",                   0,                      0,      0,      0 >>>>> }, >>>>>     { 0x16000000, >>>>> 0xfe000000,    "lu32i.d",      "r0:5,s5:20",                   0,                      0,      0,      0 >>>>> }, >>>>> -  { 0x18000000, >>>>> 0xfe000000,    "pcaddi",       "r0:5,s5:20",                   0,                      0,      0,      0 >>>>> }, >>>>> +  { 0x18000000, >>>>> 0xfe000000,    "pcaddi",       "r0:5,s5:20<<2",                0,                      0,      0,      0 >>>>> }, >>>> The Linux kernel already uses things like "pcaddi t0, 4".  To 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. >>> >>>> So I don't think we should make such a backward incompatible change >>>> without a very compelling reason.  You may argue that "<<2" has a better >>>> readability, but if we really need the readability we can write >>>> >>>> pcaddi t0, 4 # << 2 >>>> >>>> in the code anyway. >>> The immediate operand means "delta in # of instructions": >>> >>>> pcaddi t0, 4  # 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. >>> >>> 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.