yonghong-song added inline comments.
================ Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:666 + def XADDD : XADD<BPF_DW, "u64", atomic_load_add_64>; + } +} ---------------- jackmanb wrote: > FYI - I just spotted some stray `\t` in here (is it helpful to point this > out? If not let me know, I will ignore in future) \t is not allowed. I will run clang-format next time to catch such violations. Thanks for letting me know. ================ Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:684 + let Inst{47-32} = addr{15-0}; // offset + let Inst{11-8} = val; + let Inst{7-4} = Opc.Value; ---------------- jackmanb wrote: > jackmanb wrote: > > jackmanb wrote: > > > Sorry I'm a beginner with the LLVM code, could you explain what `val` > > > does? I didn't notice this when I looked through here before. > > > > > > To try and get a clue I tried just removing this line and then compiling > > > the following code: > > > > > > ```C > > > // SPDX-License-Identifier: GPL-2.0 > > > #include <stdatomic.h> > > > > > > #include <linux/bpf.h> > > > #include <bpf/bpf_helpers.h> > > > #include <bpf/bpf_tracing.h> > > > > > > __u64 test_data_64 = 0; > > > __u64 test1_result = 0; > > > > > > SEC("fentry/bpf_fentry_test1") > > > int BPF_PROG(test1, int a) > > > { > > > /* atomic_fetch_add(&test_data_64, 1); */ > > > test1_result = __sync_fetch_and_add(&test_data_64, 1); > > > return 0; > > > } > > > ``` > > > > > > And I was able to load and run the program, with the kernel on my WIP > > > branch: https://github.com/bjackman/linux-bpf/tree/wips/bpf-atomics-v0 > > > > > > The result looks like this: > > > > > > ```shell > > > $ llvm-objdump -d atomics_test.o > > > > > > atomics_test.o: file format elf64-bpf > > > > > > > > > Disassembly of section fentry/bpf_fentry_test1: > > > > > > 0000000000000000 <test1>: > > > 0: b7 01 00 00 01 00 00 00 r1 = 1 > > > 1: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0 ll > > > 3: db 12 00 00 01 00 00 00 r1 = atomic_fetch_add((u64 *)(r2 > > > + 0), PLEASE submit a bug report to https://bugs.llvm.org/ and include > > > the crash backtrace. > > > Stack dump: > > > 0. Program arguments: llvm-objdump -d atomics_test.o > > > Segmentation fault > > > ``` > > > > > > Aside from the fact that llvm-objdump crashed, the encoding `db 12 00 00 > > > 01 00 00 00` seems correct to me. If I add the `let Inst{11-8} = val` > > > back in I get `db 12 00 00 01 01 00 00` which I don't understand. > > Ah wait, I guess this is adding a 3rd operand register? In my example it's > > unclear because it is R1 which is also the dst operand. I was envisaging we > > just have semantics like `src = atomic_fetch_add(dst+off, src)` but you are > > instead proposing `dst = atomic_fetch_add(dst+off, val)`? > Sorry I mean `dst = atomic_fetch_add(src+off, val)` > Sorry I'm a beginner with the LLVM code, could you explain what `val` does? I > didn't notice this when I looked through here before. For the below statement: test1_result = __sync_fetch_and_add(&test_data_64, 1); 'val' represents the register which holds the value '1'. bit 4-7 is also used in compare-and-exchange insn as you need a memory location, in-register old/new values. > > To try and get a clue I tried just removing this line and then compiling the > following code: > > ```C > // SPDX-License-Identifier: GPL-2.0 > #include <stdatomic.h> > > #include <linux/bpf.h> > #include <bpf/bpf_helpers.h> > #include <bpf/bpf_tracing.h> > > __u64 test_data_64 = 0; > __u64 test1_result = 0; > > SEC("fentry/bpf_fentry_test1") > int BPF_PROG(test1, int a) > { > /* atomic_fetch_add(&test_data_64, 1); */ > test1_result = __sync_fetch_and_add(&test_data_64, 1); > return 0; > } > ``` > > And I was able to load and run the program, with the kernel on my WIP branch: > https://github.com/bjackman/linux-bpf/tree/wips/bpf-atomics-v0 > > The result looks like this: > > ```shell > $ llvm-objdump -d atomics_test.o > > atomics_test.o: file format elf64-bpf > > > Disassembly of section fentry/bpf_fentry_test1: > > 0000000000000000 <test1>: > 0: b7 01 00 00 01 00 00 00 r1 = 1 > 1: 18 02 00 00 00 00 00 00 00 00 00 00 00 00 00 00 r2 = 0 ll > 3: db 12 00 00 01 00 00 00 r1 = atomic_fetch_add((u64 *)(r2 + > 0), PLEASE submit a bug report to https://bugs.llvm.org/ and include the > crash backtrace. > Stack dump: > 0. Program arguments: llvm-objdump -d atomics_test.o > Segmentation fault > ``` > the crash may come from that the 'val' is not encoded properly. I will double check. > Aside from the fact that llvm-objdump crashed, the encoding `db 12 00 00 01 > 00 00 00` seems correct to me. If I add the `let Inst{11-8} = val` back in I > get `db 12 00 00 01 01 00 00` which I don't understand. in this particular case, yes, as final asm code looks like r1 = atomic_fetch_add((u64 *)(r2 + 0), r1) where the value "r1" and the result "r1" shared the same register. A little bit compiler work is need to enforce "val" register and result register always the same. My current design allows: r3 = atomic_fetch_add((u64 *)(r2 + 0), r1) and I think this is more flexible as people may later on still use "r1". But let me know whether you prefer r1 = atomic_fetch_add((u64 *)(r2 + 0), r1) always. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72184/new/ https://reviews.llvm.org/D72184 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits