Re: [PATCH v1 1/2] LoongArch: Fix pcaddi format string

[WANG Xuerui <i.swmail@xen0n.name>]

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.