Hi Alexei, Yonghong, On Tue, Jul 30, 2024 at 08:51:15PM -0700, Yonghong Song 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. > > Sounds good, I will remove the backward compat part in llvm20. Thanks for confirming! Would you mind if I fix it myself? It may affect some of the BPF code that we will be running on ARM, so we would like to get it fixed sooner. Also, I would love to gain some experience in LLVM development! Thanks, Peilin Ye
