On AIX, a libatomic supporting inline quadword atomic operations has been released, so that compatibility is not an issue now, we can enable quadword atomics by default.
Details
Details
- Reviewers
cebowleratibm hubert.reinterpretcast shchenz nemanjai - Group Reviewers
Restricted Project - Commits
- rGf26af16e2c0d: [PowerPC][AIX] Enable quadword atomics by default for AIX
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 18438–18439 | With this, turning off EnableQuadwordAtomics will have an effect for non-AIX platforms. I think this function should be left alone (except for updating the comment to say that the ability to turn off quadword atomics for AIX is a historical artifact). | |
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 18438–18439 | That's a good point. Will update the comment. | |
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 18438–18439 | I meant that perhaps the functionality should be left the same as before? Otherwise, the scope of the patch is bigger than necessary (and tests may need expansion). | |
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 18438–18439 | Is it still necessary to keep EnableQuadwordAtomics(and defaults to true) for AIX? Since the updated libatomic is already delivered, I don't see the essentiality to keep it. | |
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 18438–18439 | It is my understanding that the minimum requirements for the libatomic that has lock-free quadword atomics enabled is the same as that for Clang/LLVM on AIX in general, right? I would then agree that we may not need EnableQuadwordAtomics anymore; however, I think the patch here should update the LLVM release notes to indicate what version of the libatomic library in required. If I am not mistaken, the libatomic functionality that is needed is (at least currently) available only as part of the (redistributable) IBM Open XL runtimes. | |
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 18438–18439 |
Yes. Should be at least 64-bit AIX and power8 cpu.
Yes, the libatomic in the context is the one in IBM Open XL runtimes. Is it appropriate to mention this libatomic, which is IBM branded, in LLVM's release notes? | |
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
|---|---|---|
| 18438–18439 |
I meant whether the libatomic would install on AIX 7.2 TL5 SP3.
I don't see how it would be different than saying that you need some version of AIX or higher. | |
| llvm/docs/ReleaseNotes.rst | ||
|---|---|---|
| 204–205 | Can you add info on package name and version needed? | |
| llvm/docs/ReleaseNotes.rst | ||
|---|---|---|
| 204 | Minor nit: s/os/OS; | |
| llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
| 18439–18440 | Comment can now be removed entirely. It speaks to code that is no longer present. | |
Comment Actions
@lkail, I have no further comments (but I have not reviewed the instruction sequences generated). Can you find another reviewer to review the instruction sequences? Otherwise, if you are confident that the instruction sequences are well-tested (e.g., because libatomic dependent upon these sequences has been deployed), I am willing to approve with your confirmation.
Comment Actions
@nemanjai Much appreciated! Gentle ping @hubert.reinterpretcast. I've also tested it via compiler-rt/test/builtins/Unit/atomic_test.c which passed.
Comment Actions
As I said, I had no further comments. Please go ahead and land since the patch has been approved.
Minor nit: s/os/OS;