[PATCH] D72184: [BPF] support atomic instructions

[Yonghong Song via Phabricator via cfe-commits]

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 implemented in the kernel and what you've implemented here, which is that I used `dst` as the pointer base and `src` as the writeback register but you did the opposite.
> 
> IIUC what we have here at the moment is `dst = sync_fetch_and_add(*(src + off), dst);`. The advantage is that `dst` makes more intuitive sense as a writeback register. 
> 
> If we instead had `src = sync_fetch_and_add(*(dst + off), src)` we have the unintuitive writeback to `src`, but now a) the 2nd argument (the addend) makes more intuitive sense, and b) `dst` being the pointer base is consistent with the rest of `BPF_STX`.
could you double check? my encoding is actually
   dst = sync_fetch_and_add(*(src + off), dst);.
The encoding itself is very similar to xadd/stx.


================
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;
----------------
jackmanb wrote:
> If we go down the route of matching operands with x86 as you have done for `XFALU64` and `XCHG`, I think  we should also do it for cmpxchg.
> 
> IIUC this is `dst = atomic_cmpxchg(*(src + off), r0, new);` 
> 
> But to do it in a single x86 instruction we need to have only 2 operands + the hard-coded r0. `r0 = atomic_xmpxchg(*(dst + off), r0, src);` would seem most natural to me.
We can do this:
  r0 = atomic_xmpxchg(*(dst + off), r0, src);


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72184/new/

https://reviews.llvm.org/D72184