Messages in this thread | | | From | Huacai Chen <> | Date | Fri, 14 Oct 2022 16:58:07 +0800 | Subject | Re: [PATCH] LoongArch: BPF: Avoid declare variables in switch-case |
| |
On Fri, Oct 14, 2022 at 10:18 AM Tiezhu Yang <yangtiezhu@loongson.cn> wrote: > > > > On 10/14/2022 09:13 AM, Huacai Chen wrote: > > Hi, Xuerui, > > > > On Fri, Oct 14, 2022 at 12:43 AM WANG Xuerui <kernel@xen0n.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. OK, then I will use tmp or just tm for alignment.
> > >>> + 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 > >
| |