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

[Xi Ruoyao <xry111@xry111.site>]

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.
> 

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