[PATCH] D151312: [PowerPC][AIX] Enable quadword atomics by default for AIX

[Kai Luo via Phabricator via llvm-commits]

lkail 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();
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151312/new/

https://reviews.llvm.org/D151312