Re: [PATCH v6 4/6] target/riscv: Add support for PC-relative translation

[Richard Henderson]

On 4/4/23 00:07, LIU Zhiwei wrote:
> Yes, I think so. I just suspect whether it is easy to read and verify the correctness. And 
> the maintenance for the future.
>
>
> 1) Maybe we should split the PCREL to a split patch set, as it is a new feature. The point 
> masking can still use this thread.

Yes.

> 2) For the new patch set for PCREL, process where we need to modify one by one. One clue 
> for recognize where to modify is the ctx pc related fields, such as 
> pc_next/pc_first/succ_insn_pc.
>
> One thing may worth to try is that don't change the code in insn_trans/trans_X.  Just 
> rename the origin API we need to modify to a new name with _abs suffix. And and a 
> correspond set of API for PCREL with _pcrel suffix.
>
> For example, in DisasContext, we define
>
> void (*gen_set_gpri)(DisasContext *ctx, int reg_num, target_long imm);
>
> In disas_init_fn,
>
> if (tb_cflags(tb) & CF_PCREL) {
>      gen_set_gpri = gen_set_gpri_pcrel;
> } else {
>      gen_set_gpri = gen_set_gpri_abs;
> }
>
> Thus we can write the code in trans_insn without think about the PCREL.

No.  Please have a look at how the other conversions have progressed.  E.g.

https://lore.kernel.org/qemu-devel/20220930220312.135327-1-richard.henderson@linaro.org/

Step by step, each of the internal translation functions is converted from absolute to 
relative values.  By operating on relative values, all knowledge of "pc" is centralized, 
and it simplifies the trans_* functions.


r~