On 11/17/20 3:23 AM, Brendan Jackman via Phabricator wrote:
jackmanb added inline comments.
================
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;
----------------
yonghong-song wrote:
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.
Got it - this design will introduce some impedance mismatch with x86, since
XADD has only two operands (`r3 = atomic_fetch_add((u64 *)(r2 + 0), r1)` cannot
be implemented in a single instruction). It also hard-codes RAX as the third
operand for [cmp]xchg so my current kernel implementation hard-codes R0 -
that's also what Alexei supported in the separate email thread.
Will you be available to discuss this in the BPF office hours this week? There
are a few possible ways forward, we just need to decide which tradeoff is best.
Yes, I am available. We can discuss on Thursday morning then. We can
certainly put constraints to how to assign registers.
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
