On 7/30/24 6:19 PM, Alexei Starovoitov wrote:
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.
Sounds good, I will remove the backward compat part in llvm20.
