On 7/31/24 1:44 PM, Peilin Ye wrote:
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!
Peilin, when I saw your email, I have almost done with the change.
The below is the llvm patch:
https://github.com/llvm/llvm-project/pull/101428
Please help take a look. You are certainly welcome to do llvm
related work. Just respond earlier to mention you intend to do
a particular llvm patch and we are happy for you to contribute
and will help when you have any questions.
Thanks,
Peilin Ye
