Re: [PATCH v1 1/2] LoongArch: BPF: Clean up and decouple is32 parameter from move_imm()

--===============2028487587190646803==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit

> diff --git a/arch/loongarch/net/bpf_jit.h b/arch/loongarch/net/bpf_jit.h
> index a8e29be35fa8..9438fce49e69 100644
> --- a/arch/loongarch/net/bpf_jit.h
> +++ b/arch/loongarch/net/bpf_jit.h
> @@ -137,7 +137,7 @@ static inline void move_addr(struct jit_ctx *ctx, enum loongarch_gpr rd, u64 add
>  	emit_insn(ctx, lu52id, rd, rd, imm_63_52);
>  }
>
> -static inline void move_imm(struct jit_ctx *ctx, enum loongarch_gpr rd, long imm, bool is32)
> +static inline void move_imm(struct jit_ctx *ctx, enum loongarch_gpr rd, long imm)
>  {
>  	long imm_11_0, imm_31_12, imm_51_32, imm_63_52, imm_51_0, imm_51_31;
>
> @@ -150,7 +150,7 @@ static inline void move_imm(struct jit_ctx *ctx, enum loongarch_gpr rd, long imm
>  	/* addiw rd, $zero, imm_11_0 */
>  	if (is_signed_imm12(imm)) {
>  		emit_insn(ctx, addiw, rd, LOONGARCH_GPR_ZERO, imm);
> -		goto zext;
> +		return;
>  	}
>
>  	/* ori rd, $zero, imm_11_0 */
>  	if (is_unsigned_imm12(imm)) {
>  		emit_insn(ctx, ori, rd, LOONGARCH_GPR_ZERO, imm);
>  		goto zext;
                     ^^^^^^^^^^

Does this code compile? The is_signed_imm12 branch now returns early, but
the is_unsigned_imm12 branch still jumps to the zext label that was removed
by the same commit:

> @@ -195,9 +195,6 @@ static inline void move_imm(struct jit_ctx *ctx, enum loongarch_gpr rd, long imm
>  		if (!is_signed_imm52(imm))
>  			emit_insn(ctx, lu52id, rd, rd, imm_63_52);
>  	}
> -
> -zext:
> -	emit_zext_32(ctx, rd, is32);
>  }

This dangling goto zext; at the is_unsigned_imm12 path has no target after
the label was deleted. Should this become 'return;' to match the
is_signed_imm12 path?

[ ... ]

> diff --git a/arch/loongarch/net/bpf_jit.c b/arch/loongarch/net/bpf_jit.c
> index 2738b4db1165..d676936e5b98 100644
> --- a/arch/loongarch/net/bpf_jit.c
> +++ b/arch/loongarch/net/bpf_jit.c

[ ... ]

> @@ -765,7 +765,8 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
>  	/* dst = imm */
>  	case BPF_ALU | BPF_MOV | BPF_K:
>  	case BPF_ALU64 | BPF_MOV | BPF_K:
> -		move_imm(ctx, dst, imm, is32);
> +		move_imm(ctx, dst, imm);
> +		emit_zext_32(ctx, dst, is32);
>  		break;

[ ... ]

> @@ -843,12 +847,13 @@ static int build_insn(const struct bpf_insn *insn, struct jit_ctx *ctx, bool ext
>  	case BPF_ALU | BPF_DIV | BPF_K:
>  	case BPF_ALU64 | BPF_DIV | BPF_K:
>  		if (!off) {
> -			move_imm(ctx, t1, imm, is32);
> +			move_imm(ctx, t1, imm);
> +			emit_zext_32(ctx, t1, is32);

All these call sites were updated to the three-argument form (as expected
after move_imm changed from taking four parameters to three). However:

build_insn() has a BPF_NEG case just before the BPF_AND case. Can you check
whether it was updated? Looking at the diff hunks: the BPF_MOD | BPF_K case
ends near line 895, and the next hunk starts at BPF_AND | BPF_K near line
918. The BPF_ALU/BPF_ALU64 | BPF_NEG cases fall between these two hunks, so
they would not appear in the diff if they were left unchanged. If the NEG
case still has 'move_imm(ctx, t1, imm, is32);' with four arguments, that
would be a function call with too many arguments and fail to compile.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/30326478489
--===============2028487587190646803==--