| Message ID | 1447195301-16757-2-git-send-email-yang.shi@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Nov 10, 2015 at 2:41 PM, Yang Shi <yang.shi@linaro.org> wrote: > aarch64 doesn't have native store immediate instruction, such operation Actually, aarch64 does have "STR (immediate)". For arm64 JIT, we can consider using it as an optimization. You may also want to consider adding a note about the corresponding test cases: commit cffc642d93f9 ("test_bpf: add 173 new testcases for eBPF"). Otherwise, the patch below looks good. Reviewed-by: Zi Shen Lim <zlim.lnx@gmail.com> > has to be implemented by the below instruction sequence: > > Load immediate to register > Store register > > Signed-off-by: Yang Shi <yang.shi@linaro.org> > CC: Zi Shen Lim <zlim.lnx@gmail.com> > CC: Xi Wang <xi.wang@gmail.com> > --- > arch/arm64/net/bpf_jit_comp.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c > index 6809647..49c1f1b 100644 > --- a/arch/arm64/net/bpf_jit_comp.c > +++ b/arch/arm64/net/bpf_jit_comp.c > @@ -563,7 +563,25 @@ emit_cond_jmp: > case BPF_ST | BPF_MEM | BPF_H: > case BPF_ST | BPF_MEM | BPF_B: > case BPF_ST | BPF_MEM | BPF_DW: > - goto notyet; > + /* Load imm to a register then store it */ > + ctx->tmp_used = 1; > + emit_a64_mov_i(1, tmp2, off, ctx); > + emit_a64_mov_i(1, tmp, imm, ctx); > + switch (BPF_SIZE(code)) { > + case BPF_W: > + emit(A64_STR32(tmp, dst, tmp2), ctx); > + break; > + case BPF_H: > + emit(A64_STRH(tmp, dst, tmp2), ctx); > + break; > + case BPF_B: > + emit(A64_STRB(tmp, dst, tmp2), ctx); > + break; > + case BPF_DW: > + emit(A64_STR64(tmp, dst, tmp2), ctx); > + break; > + } > + break; > > /* STX: *(size *)(dst + off) = src */ > case BPF_STX | BPF_MEM | BPF_W: > -- > 2.0.2 >
On Tue, Nov 10, 2015 at 06:45:39PM -0800, Z Lim wrote: > On Tue, Nov 10, 2015 at 2:41 PM, Yang Shi <yang.shi@linaro.org> wrote: > > aarch64 doesn't have native store immediate instruction, such operation > > Actually, aarch64 does have "STR (immediate)". For arm64 JIT, we can > consider using it as an optimization. Yes, I'd definitely like to see that in preference to moving via a temporary register. Will
On Wed, Nov 11, 2015 at 12:12:56PM +0000, Will Deacon wrote: > On Tue, Nov 10, 2015 at 06:45:39PM -0800, Z Lim wrote: > > On Tue, Nov 10, 2015 at 2:41 PM, Yang Shi <yang.shi@linaro.org> wrote: > > > aarch64 doesn't have native store immediate instruction, such operation > > > > Actually, aarch64 does have "STR (immediate)". For arm64 JIT, we can > > consider using it as an optimization. > > Yes, I'd definitely like to see that in preference to moving via a > temporary register. Wait a second, we're both talking rubbish here :) The STR (immediate) form is referring to the addressing mode, whereas this patch wants to store an immediate value to memory, which does need moving to a register first. So the original patch is fine. Will
On 11/11/2015 4:39 AM, Will Deacon wrote: > On Wed, Nov 11, 2015 at 12:12:56PM +0000, Will Deacon wrote: >> On Tue, Nov 10, 2015 at 06:45:39PM -0800, Z Lim wrote: >>> On Tue, Nov 10, 2015 at 2:41 PM, Yang Shi <yang.shi@linaro.org> wrote: >>>> aarch64 doesn't have native store immediate instruction, such operation >>> >>> Actually, aarch64 does have "STR (immediate)". For arm64 JIT, we can >>> consider using it as an optimization. >> >> Yes, I'd definitely like to see that in preference to moving via a >> temporary register. > > Wait a second, we're both talking rubbish here :) The STR (immediate) > form is referring to the addressing mode, whereas this patch wants to > store an immediate value to memory, which does need moving to a register > first. Yes, the immediate means immediate offset for addressing index. Doesn't mean to store immediate to memory. I don't think any load-store architecture has store immediate instruction. Thanks, Yang > > So the original patch is fine. > > Will >
On Thu, Nov 12, 2015 at 11:33 AM, Shi, Yang <yang.shi@linaro.org> wrote: > On 11/11/2015 4:39 AM, Will Deacon wrote: >> >> Wait a second, we're both talking rubbish here :) The STR (immediate) >> form is referring to the addressing mode, whereas this patch wants to >> store an immediate value to memory, which does need moving to a register >> first. > > > Yes, the immediate means immediate offset for addressing index. Doesn't mean > to store immediate to memory. > > I don't think any load-store architecture has store immediate instruction. > Indeed. Sorry for the noise. Somehow Will caught a whiff of whatever I was smoking then :)
Hi folks, Any more comments on this patch (store immediate only)? I need more time to add XADD (I'm supposed everyone agrees it is equivalent to atomic_add). However, this one is irrelevant to XADD, so we may be able to apply it first? Thanks, Yang On 11/12/2015 7:45 PM, Z Lim wrote: > On Thu, Nov 12, 2015 at 11:33 AM, Shi, Yang <yang.shi@linaro.org> wrote: >> On 11/11/2015 4:39 AM, Will Deacon wrote: >>> >>> Wait a second, we're both talking rubbish here :) The STR (immediate) >>> form is referring to the addressing mode, whereas this patch wants to >>> store an immediate value to memory, which does need moving to a register >>> first. >> >> >> Yes, the immediate means immediate offset for addressing index. Doesn't mean >> to store immediate to memory. >> >> I don't think any load-store architecture has store immediate instruction. >> > > Indeed. Sorry for the noise. > > Somehow Will caught a whiff of whatever I was smoking then :) >
diff --git a/arch/arm64/net/bpf_jit_comp.c b/arch/arm64/net/bpf_jit_comp.c index 6809647..49c1f1b 100644 --- a/arch/arm64/net/bpf_jit_comp.c +++ b/arch/arm64/net/bpf_jit_comp.c @@ -563,7 +563,25 @@ emit_cond_jmp: case BPF_ST | BPF_MEM | BPF_H: case BPF_ST | BPF_MEM | BPF_B: case BPF_ST | BPF_MEM | BPF_DW: - goto notyet; + /* Load imm to a register then store it */ + ctx->tmp_used = 1; + emit_a64_mov_i(1, tmp2, off, ctx); + emit_a64_mov_i(1, tmp, imm, ctx); + switch (BPF_SIZE(code)) { + case BPF_W: + emit(A64_STR32(tmp, dst, tmp2), ctx); + break; + case BPF_H: + emit(A64_STRH(tmp, dst, tmp2), ctx); + break; + case BPF_B: + emit(A64_STRB(tmp, dst, tmp2), ctx); + break; + case BPF_DW: + emit(A64_STR64(tmp, dst, tmp2), ctx); + break; + } + break; /* STX: *(size *)(dst + off) = src */ case BPF_STX | BPF_MEM | BPF_W:
aarch64 doesn't have native store immediate instruction, such operation has to be implemented by the below instruction sequence: Load immediate to register Store register Signed-off-by: Yang Shi <yang.shi@linaro.org> CC: Zi Shen Lim <zlim.lnx@gmail.com> CC: Xi Wang <xi.wang@gmail.com> --- arch/arm64/net/bpf_jit_comp.c | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-)