[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ae6260e5-0eb6-a615-7032-6481cd186f3f@loongson.cn>
Date: Fri, 14 Oct 2022 10:18:38 +0800
From: Tiezhu Yang <yangtiezhu@...ngson.cn>
To: Huacai Chen <chenhuacai@...nel.org>,
WANG Xuerui <kernel@...0n.name>
Cc: Huacai Chen <chenhuacai@...ngson.cn>, loongarch@...ts.linux.dev,
Xuefeng Li <lixuefeng@...ngson.cn>,
Guo Ren <guoren@...nel.org>,
Jiaxun Yang <jiaxun.yang@...goat.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] LoongArch: BPF: Avoid declare variables in switch-case
On 10/14/2022 09:13 AM, Huacai Chen wrote:
> Hi, Xuerui,
>
> On Fri, Oct 14, 2022 at 12:43 AM WANG Xuerui <kernel@...0n.name> wrote:
>>
>> On 10/13/22 23:40, Huacai Chen wrote:
>>> Not all compilers support declare variables in switch-case, so move
>>> declarations to the beginning of a function. Otherwise we may get such
>>> build errors:
...
>>>
>>> static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool extra_pass)
>>> {
>>> - const bool is32 = BPF_CLASS(insn->code) == BPF_ALU ||
>>> - BPF_CLASS(insn->code) == BPF_JMP32;
>>> + u8 t0 = -1;
>> Here "t0" seems to be a versatile temp value, while the "t1" below is
>> the actual GPR $t1. What about renaming "t0" to something like "tmp" to
>> reduce confusion? I believe due to things like "t0 = LOONGARCH_GPR_ZERO"
>> the "t0" is 100% not an actual mapping to $t0.
> I rename t7 to t0 because there is no t3-t6, t7 looks very strange.
> But from emit_cond_jmp() the 3rd and 4th parameters have no difference
> so I suppose t0 is just OK, then whether rename it to tmp depends on
> Tiezhu's opinion.
>
Use "tmp" seems better due to it is a temp value.
>>> + u64 func_addr;
>>> + bool func_addr_fixed;
>>> + int i = insn - ctx->prog->insnsi;
>>> + int ret, jmp_offset;
>>> const u8 code = insn->code;
>>> const u8 cond = BPF_OP(code);
>>> const u8 t1 = LOONGARCH_GPR_T1;
>>> @@ -400,8 +402,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
>>> const u8 dst = regmap[insn->dst_reg];
>>> const s16 off = insn->off;
>>> const s32 imm = insn->imm;
>>> - int jmp_offset;
>>> - int i = insn - ctx->prog->insnsi;
>>> + const u64 imm64 = (u64)(insn + 1)->imm << 32 | (u32)insn->imm;
>>> + const bool is32 = BPF_CLASS(insn->code) == BPF_ALU || BPF_CLASS(insn->code) == BPF_JMP32;
>> Please consider reducing diff damage and not touching parts not directly
>> affected by this change. For example this "is32" declaration and
>> initialization was moved although not related to this change.
It looks reasonable, one change per patch is better.
> I think defining variables from simple to complex and grouping them
> can make life easier. :)
>
No strong opinion on this, I am OK either way.
Thanks,
Tiezhu
Powered by blists - more mailing lists