[PATCH] D151312: [PowerPC][AIX] Enable quadword atomics by default for AIX
Hubert Tong via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jul 4 19:43:17 PDT 2023
hubert.reinterpretcast added inline comments.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:18411
bool PPCTargetLowering::shouldInlineQuadwordAtomics() const {
- // TODO: 16-byte atomic type support for AIX is in progress; we should be able
- // to inline 16-byte atomic ops on AIX too in the future.
- return Subtarget.isPPC64() &&
- (EnableQuadwordAtomics || !Subtarget.getTargetTriple().isOSAIX()) &&
+ return Subtarget.isPPC64() && EnableQuadwordAtomics &&
Subtarget.hasQuadwordAtomics();
----------------
lkail wrote:
> hubert.reinterpretcast wrote:
> > lkail wrote:
> > > hubert.reinterpretcast wrote:
> > > > lkail wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > 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).
> > > > > That's a good point. Will update the comment.
> > > > 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).
> > > 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.
> > 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.
> > 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?
>
> Yes. Should be at least 64-bit AIX and power8 cpu.
>
> > the libatomic functionality that is needed is (at least currently) available only as part of the (redistributable) IBM Open XL runtimes.
>
> 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?
> > 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?
>
> Yes. Should be at least 64-bit AIX and power8 cpu.
I meant whether the `libatomic` would install on AIX 7.2 TL5 SP3.
> > the libatomic functionality that is needed is (at least currently) available only as part of the (redistributable) IBM Open XL runtimes.
>
> 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?
I don't see how it would be different than saying that you need some version of AIX or higher.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D151312/new/
https://reviews.llvm.org/D151312
More information about the llvm-commits
mailing list