Re: [PATCH 2/2] tcg/loongarch64: Add direct jump support

[Qi Hu]

On 2022/10/12 19:34, WANG Xuerui wrote:
> Hi,
>
> Thanks for the improvement! Some room for improvement though...
>
> On 2022/10/12 17:13, Qi Hu wrote:
> > Similar to the ARM64, LoongArch has PC-relative instructions such as
> > PCADDU18I. These instructions can be used to support direct jump for
> > LoongArch. Additionally, if instruction "B offset" can cover the target
> > address, "tb_target_set_jmp_target" will only patch the "B offset".
>
> "if the target is within +/- 28 bits range, a single "B offset" plus a 
> nop will be used instead" might sound better?
Yeah, I will add this at commit message. :)
> > Signed-off-by: Qi Hu <huqi@loongson.cn>
> > ---
> >   tcg/loongarch64/tcg-insn-defs.c.inc |  3 ++
> >   tcg/loongarch64/tcg-target.c.inc    | 49 ++++++++++++++++++++++++++---
> >   tcg/loongarch64/tcg-target.h        |  2 +-
> >   3 files changed, 48 insertions(+), 6 deletions(-)
> >
> > diff --git a/tcg/loongarch64/tcg-insn-defs.c.inc 
> > b/tcg/loongarch64/tcg-insn-defs.c.inc
> > index d162571856..f5869c6bb1 100644
> > --- a/tcg/loongarch64/tcg-insn-defs.c.inc
> > +++ b/tcg/loongarch64/tcg-insn-defs.c.inc
> > @@ -112,6 +112,9 @@ typedef enum {
> >       OPC_BLE = 0x64000000,
> >       OPC_BGTU = 0x68000000,
> >       OPC_BLEU = 0x6c000000,
> > +    /* pseudo-instruction */
> > +    NOP = 0x03400000,
> > +
>
> You certainly saw the big fat comment block saying the file was 
> auto-generated and thus shouldn't be touched, didn't you? ;-)
>
> I saw your need for a NOP constant later though, and you can instead 
> add `#define OPC_NOP OPC_ANDI` in tcg-target.c.inc, just below the 
> include of tcg-insn-defs.c.inc.
Oh, I think I can add  "tcg_out_nop" instead of "NOP" here.
> >   } LoongArchInsn;
> >     static int32_t __attribute__((unused))
> > diff --git a/tcg/loongarch64/tcg-target.c.inc 
> > b/tcg/loongarch64/tcg-target.c.inc
> > index f5a214a17f..3a7b1df081 100644
> > --- a/tcg/loongarch64/tcg-target.c.inc
> > +++ b/tcg/loongarch64/tcg-target.c.inc
> > @@ -1058,11 +1058,24 @@ static void tcg_out_op(TCGContext *s, 
> > TCGOpcode opc,
> >           break;
> >         case INDEX_op_goto_tb:
> > -        assert(s->tb_jmp_insn_offset == 0);
> > -        /* indirect jump method */
> > -        tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TMP0, TCG_REG_ZERO,
> > -                   (uintptr_t)(s->tb_jmp_target_addr + a0));
> > -        tcg_out_opc_jirl(s, TCG_REG_ZERO, TCG_REG_TMP0, 0);
> > +        if (s->tb_jmp_insn_offset != NULL) {
> > +            /* TCG_TARGET_HAS_direct_jump */
> > +            /* Ensure that PCADD+JIRL are 8-byte aligned so that an 
> > atomic
> > +               write can be used to patch the target address. */
> There isn't a "PCADD" in LoongArch, and it's possible for the 2 insns 
> to be "B + NOP" as well. So better reword a bit like "Ensure that the 
> 8-byte direct jump fragment is aligned ..." (and add another space 
> after the period at the end of the sentence).
~
> > +            if ((uintptr_t)s->code_ptr & 7) {
> > +                tcg_out32(s, NOP);
> > +            }
> > +            s->tb_jmp_insn_offset[a0] = tcg_current_code_size(s);
> > +            /* actual branch destination will be patched by
> > +               tb_target_set_jmp_target later. */
> Either make it a proper sentence by title-casing "actual" and adding 
> another space after the trailing period, or remove the period.
I will modify these comments.
> > +            tcg_out_opc_pcaddu18i(s, TCG_REG_TMP0, 0);
> > +            tcg_out_opc_jirl(s, TCG_REG_ZERO, TCG_REG_TMP0, 0);
> > +        } else {
> > +            /* !TCG_TARGET_HAS_direct_jump */
> > +            tcg_out_ld(s, TCG_TYPE_PTR, TCG_REG_TMP0, TCG_REG_ZERO,
> > +                    (uintptr_t)(s->tb_jmp_target_addr + a0));
> > +            tcg_out_opc_jirl(s, TCG_REG_ZERO, TCG_REG_TMP0, 0);
> > +        }
> We unconditionally support the direct jump method after the change, so 
> do we need to retain this block any more? Note the aarch64 port 
> currently does the same (declaring unconditional support for direct 
> jumps but keeping both code paths), if we want to remove this code 
> path then you may want to remove the aarch64 one respectively too.
Yes, maybe we can remove these in our patch and submit another patch to 
modify aarch64 port?
> >           set_jmp_reset_offset(s, a0);
> >           break;
> >   @@ -1708,6 +1721,32 @@ static void tcg_target_init(TCGContext *s)
> >       tcg_regset_set_reg(s->reserved_regs, TCG_REG_RESERVED);
> >   }
> >   +void tb_target_set_jmp_target(uintptr_t tc_ptr, uintptr_t jmp_rx,
> > +                              uintptr_t jmp_rw, uintptr_t addr)
> > +{
>
> Move the function to somewhere above, like above the "/* Entrypoints 
> */" section (and maybe introduce another section)? The various parts 
> are more-or-less arranged in a particular order, so it's like going 
> back to implementing concrete things after finishing everything and 
> calling it a day.
~
> Also you forgot to remove the now inappropriate comment about this 
> helper in tcg-target.h.
Oh, I will remove the comment.
> > +    tcg_insn_unit i1, i2;
> > +
> > +    ptrdiff_t offset = addr - jmp_rx;
> > +
> > +    if (offset == sextreg(offset, 0, 28)) {
> > +        i1 = OPC_B | ((offset >> 18) & 0x3ff) | ((offset << 8) & 
> > 0x3fffc00);
> Use encode_sd10k16_insn instead. No need to juggle the bits yourself 
> and you get nice range assertion for free.
~
> > +        i2 = NOP;
> > +    } else {
> > +        offset >>= 2;
> > +
> > +        ptrdiff_t upper, lower;
> Promote the declaration to the top of the block (before offset 
> shifting), IIRC that's the official coding style.
~
> > +        upper = ((offset + (1 << 15)) >> 16) & 0xfffff;
> > +        lower = (offset & 0xffff);
> > +        /* patch pcaddu18i */
> > +        i1 = OPC_PCADDU18I | upper << 5 | TCG_REG_T0;
> > +        /* patch jirl */
> > +        i2 = OPC_JIRL | lower << 10 | TCG_REG_T0 << 5;
>
> Similarly you could simplify the assembly here, with encode_dsj20_insn 
> and encode_djsk16_insn respectively.
>
> And the code is straight-forward enough to not need the "patch foo" 
> comments.
~
> > +    }
> > +    uint64_t pair = (uint64_t)i2 << 32 | i1;
> Maybe add a couple more parentheses to make the precedence easier to 
> follow? (for people who struggle to remember things like this, like me)
~
> > +    qatomic_set((uint64_t *)jmp_rw, pair);
> > +    flush_idcache_range(jmp_rx, jmp_rw, 8);
> > +}
> > +
> >   typedef struct {
> >       DebugFrameHeader h;
> >       uint8_t fde_def_cfa[4];
> > diff --git a/tcg/loongarch64/tcg-target.h b/tcg/loongarch64/tcg-target.h
> > index 67380b2432..0e552731f5 100644
> > --- a/tcg/loongarch64/tcg-target.h
> > +++ b/tcg/loongarch64/tcg-target.h
> > @@ -123,7 +123,7 @@ typedef enum {
> >   #define TCG_TARGET_HAS_clz_i32          1
> >   #define TCG_TARGET_HAS_ctz_i32          1
> >   #define TCG_TARGET_HAS_ctpop_i32        0
> > -#define TCG_TARGET_HAS_direct_jump      0
> > +#define TCG_TARGET_HAS_direct_jump      1
> >   #define TCG_TARGET_HAS_brcond2          0
> >   #define TCG_TARGET_HAS_setcond2         0
> >   #define TCG_TARGET_HAS_qemu_st8_i32     0

Thanks for your advise. I will make some changes and submit 2nd version.

Qi