[PATCH] D72184: [BPF] support atomic instructions

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG286daafd6512: [BPF] support atomic instructions (authored by yonghong-song). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 309167. yonghong-song added a comment. fix clang-format warnings. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72184/new/ https://reviews.llvm.org/D72184 Files: llvm/lib/Target/BPF/BPFInstrFormats.td

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 308891. yonghong-song added a comment. use llvm_unreachable() in default case of the switch statement Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72184/new/ https://reviews.llvm.org/D72184 Files:

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 308884. yonghong-song edited the summary of this revision. yonghong-song added a comment. - remove -mcpu=v4. - for new instructions (except xadd), for 32bit mode, only alu32 mode is supported. I chose this way since we have -mcpu=v3 for a while which

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFMIChecking.cpp:199 + unsigned newOpcode; + switch(MI.getOpcode()) { + case BPF::XFADDW32: newOpcode = BPF::XADDW32; break; yonghong-song wrote: > yonghong-song wrote: > > ast wrote: > >

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added inline comments. Comment at: llvm/lib/Target/BPF/BPFMIChecking.cpp:199 + unsigned newOpcode; + switch(MI.getOpcode()) { + case BPF::XFADDW32: newOpcode = BPF::XADDW32; break; yonghong-song wrote: > ast wrote: > > With this

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added inline comments. Comment at: llvm/lib/Target/BPF/BPFMIChecking.cpp:199 + unsigned newOpcode; + switch(MI.getOpcode()) { + case BPF::XFADDW32: newOpcode = BPF::XADDW32; break; ast wrote: > With this logic in place Andrii has a

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFMIChecking.cpp:199 + unsigned newOpcode; + switch(MI.getOpcode()) { + case BPF::XFADDW32: newOpcode = BPF::XADDW32; break; With this logic in place Andrii has a point. There is no need

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 308517. yonghong-song edited the summary of this revision. yonghong-song added a comment. - remove atomic_fetch_sub which can be implemented with neg + atomic_fetch_add - add support for xand, xor, xxor (xadd already been supported) - for any given

[PATCH] D72184: [BPF] support atomic instructions

jackmanb added a comment. > I did not see kernel has atomic_store, do you mean atomic_set? Sorry yep I meant `atomic_set` > Do you suggest we also implement atomic_set? There is no need for 64-bit > architecture like x64, right? Yeah actually now I think about it, `atomic_set` is pretty

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added a comment. Ya, the above llvm crash is expected as bpf backend does not handle AtomicStore. For kernel code, I can see: kvm/x86.c: vcpu->arch.nmi_pending += atomic_xchg(>arch.nmi_queued, 0); ... kvm/x86.c: atomic_set(_guest_has_master_clock, 1); So

[PATCH] D72184: [BPF] support atomic instructions

jackmanb added a comment. BTW to investigate my previous comment I tried compiling this code: // SPDX-License-Identifier: GPL-2.0 #include #include #include __u64 add64_value = 1; __u64 add64_result; __u32 add32_value = 1; __u32 add32_result; __u64 add_stack_value_copy;

[PATCH] D72184: [BPF] support atomic instructions

jackmanb added a comment. I thought a little more about something I was saying in the office hours. I'm pretty sure GCC's `__atomic_store(, , order)` should fail to compile for anything other than `order=__ATOMIC_RELAXED`, since with the current kernel patchset we have `BPF_SET` (which is

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 306598. yonghong-song added a comment. add some comments in test w.r.t. __sync_lock_test_and_set() Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72184/new/ https://reviews.llvm.org/D72184 Files:

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added a comment. okay, my fault. I must have uploaded an old version of the patch so comment is not in the test. will upload the new version tonight. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72184/new/

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added a comment. In D72184#2407264 , @ast wrote: > Looks like the test didn't change. Only commit log... that's fine. I think > the diff is ready to land, but let's wait for the kernel patches to be ready > as well. I added some comments

[PATCH] D72184: [BPF] support atomic instructions

ast added a comment. Looks like the test didn't change. Only commit log... that's fine. I think the diff is ready to land, but let's wait for the kernel patches to be ready as well. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72184/new/

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 306592. yonghong-song edited the summary of this revision. yonghong-song added a comment. add proper comment in test and clarify in commit message for __sync_lock_test_and_set which actually means an atomic exchange operation. Repository: rG LLVM

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/test/CodeGen/BPF/atomics_2.ll:14 +; int test_xchg_64(long *p, long v) { +; return __sync_lock_test_and_set(p, v); +; } yonghong-song wrote: > ast wrote: > > test_and_set is not the same as xchg. > > xchg

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added inline comments. Comment at: llvm/test/CodeGen/BPF/atomics_2.ll:14 +; int test_xchg_64(long *p, long v) { +; return __sync_lock_test_and_set(p, v); +; } ast wrote: > test_and_set is not the same as xchg. > xchg doesn't do comparison.

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/test/CodeGen/BPF/atomics_2.ll:14 +; int test_xchg_64(long *p, long v) { +; return __sync_lock_test_and_set(p, v); +; } test_and_set is not the same as xchg. xchg doesn't do comparison. Repository: rG LLVM

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 306485. yonghong-song edited the summary of this revision. yonghong-song added a comment. - remove all char/short atomic operations - remove barrier instruction Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added a comment. Thanks. Somehow my build is successful. I kind of know what is the possible issue but wonder why I did not hit it. > Can we please keep barriers out of scope? I think there's a lot of design to > be done there and I'd rather just get the core atomics working

[PATCH] D72184: [BPF] support atomic instructions

jackmanb added a comment. Can we please keep barriers out of scope? I think there's a lot of design to be done there and I'd rather just get the core atomics working first. BTW I got [ 31%] Building LanaiGenDAGISel.inc...

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added inline comments. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:783 + let Inst{47-32} = addr{15-0}; // offset + let Inst{11-8} = new; + let Inst{7-4} = BPF_CMPXCHG.Value; ast wrote: > yonghong-song wrote: > > jackmanb wrote: > > > If we

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:783 + let Inst{47-32} = addr{15-0}; // offset + let Inst{11-8} = new; + let Inst{7-4} = BPF_CMPXCHG.Value; yonghong-song wrote: > jackmanb wrote: > > If we go down the route of

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 306172. yonghong-song edited the summary of this revision. yonghong-song added a comment. add support for a barrier function. The correspond C code is __sync_synchronize(). If we want to have variant like barrier for read/write/rw, etc. we may need to

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 306146. yonghong-song edited the summary of this revision. yonghong-song added a comment. add fetch_and_{and,or,xor} support force cmpxchg insn return results in r0 like r0 = cmpxchg(addr, r0, new) Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added inline comments. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:699 + let Inst{51-48} = addr{19-16}; // base reg + let Inst{55-52} = dst; + let Inst{47-32} = addr{15-0}; // offset jackmanb wrote: > There is another mismatch between what I

[PATCH] D72184: [BPF] support atomic instructions

jackmanb added inline comments. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:699 + let Inst{51-48} = addr{19-16}; // base reg + let Inst{55-52} = dst; + let Inst{47-32} = addr{15-0}; // offset There is another mismatch between what I implemented in the

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added a comment. make sense. will add and/or/xor support in the next revision. will also think about how to support hardware-level barrier. Totally agree that we should have adequate atomic op support in cpu=v4. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72184: [BPF] support atomic instructions

ast added a comment. looks good. Before landing we need to agree on the full set of instructions that -mcpu=v4 will support. atomic_fetch_or|xor|and are probably needed as instructions. The kernel JIT will generate x86 cmpxchg for them. Because if llvm generates bpf cmpxchg insn then we'd need

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 305895. yonghong-song added a comment. use the same register for dst and val for fetch-add, fetch-sub and xchg instructions. for fetch-sub, if it is deemed later using different registers for dst/val is preferred, I can make the change then.

[PATCH] D72184: [BPF] support atomic instructions

ast 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; jackmanb wrote: > yonghong-song wrote: > > jackmanb wrote: > > > jackmanb wrote: > >

Re: [PATCH] D72184: [BPF] support atomic instructions

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

[PATCH] D72184: [BPF] support atomic instructions

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:

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added inline comments. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:666 +def XADDD : XADD; + } +} 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

[PATCH] D72184: [BPF] support atomic instructions

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; jackmanb wrote: > jackmanb wrote: > > Sorry I'm a beginner with the LLVM code,

[PATCH] D72184: [BPF] support atomic instructions

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; jackmanb wrote: > Sorry I'm a beginner with the LLVM code, could you explain

[PATCH] D72184: [BPF] support atomic instructions

jackmanb added a comment. Sorry I was disrupted and not able to work on this last week! I've just got started trying to integrate this with my kernel patches. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:666 +def XADDD : XADD; + } +} FYI - I just

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song added inline comments. Comment at: llvm/test/CodeGen/BPF/atomics_2.ll:124 +; CHECK: r0 = r2 +; CHECK: r0 = cmpxchg_64(r1 + 0, r0, r3) +; CHECK: encoding: [0xdb,0x01,0x00,0x00,0xf1,0x03,0x00,0x00] ast wrote: > Looks like it's generating correct code

[PATCH] D72184: [BPF] support atomic instructions

ast added inline comments. Comment at: llvm/test/CodeGen/BPF/atomics_2.ll:124 +; CHECK: r0 = r2 +; CHECK: r0 = cmpxchg_64(r1 + 0, r0, r3) +; CHECK: encoding: [0xdb,0x01,0x00,0x00,0xf1,0x03,0x00,0x00] Looks like it's generating correct code without special

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song marked an inline comment as done. yonghong-song added inline comments. Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:830 + +let Predicates = [BPFHasAtomicExt] in { + def CMPXCHGD : CMPXCHG; ast wrote: > let Defs = [R0], Uses = [R0] > and

[PATCH] D72184: [BPF] support atomic instructions

yonghong-song updated this revision to Diff 302617. yonghong-song retitled this revision from "[WIP][BPF] support exchange/compare-and-exchange instruction" to "[BPF] support atomic instructions". yonghong-song edited the summary of this revision. yonghong-song added a comment. - remove RFC