On Mon, Jul 29, 2024 at 10:14 PM Yonghong Song <yonghong.song@xxxxxxxxx> wrote: > > > This sounds like a compiler bug. > > > > Yonghong, Jose, > > do you know what compilers do for other backends? > > Is it allowed to convert sycn_fetch_add into sync_add when fetch part is unused? > > This behavior is introduced by the following llvm commit: > https://github.com/llvm/llvm-project/commit/286daafd65129228e08a1d07aa4ca74488615744 > > Specifically the following commit message: > > ======= > Similar to xadd, atomic xadd, xor and xxor (atomic_<op>) > instructions are added for atomic operations which do not > have return values. LLVM will check the return value for > __sync_fetch_and_{add,and,or,xor}. > If the return value is used, instructions atomic_fetch_<op> > will be used. Otherwise, atomic_<op> instructions will be used. So it's a bpf backend bug. Great. That's fixable. Would have been much harder if this transformation was performed by the middle end. > ====== > > Basically, if no return value, __sync_fetch_and_add() will use > xadd insn. The decision is made at that time to maintain backward compatibility. > For one example, in bcc > https://github.com/iovisor/bcc/blob/master/src/cc/export/helpers.h#L1444 > we have > #define lock_xadd(ptr, val) ((void)__sync_fetch_and_add(ptr, val)) > > Should we use atomic_fetch_*() always regardless of whether the return > val is used or not? Probably, it should still work. Not sure what gcc > does for this case. Right. We did it for backward compat. Older llvm was completely wrong to generate xadd for __sync_fetch_and_add. That was my hack from 10 years ago when xadd was all we had. So we fixed that old llvm bug, but introduced another with all good intentions. Since proper atomic insns were introduced 3 years ago we should remove this backward compat feature/bug from llvm. The only breakage is for kernels older than 5.12. I think that's an acceptable risk.
