[PATCH] D122377: [PowerPC] Support 16-byte lock free atomics on pwr8 and up

[Kai Luo via Phabricator via llvm-commits]

lkail marked an inline comment as done.
lkail added inline comments.


================
Comment at: clang/lib/Basic/Targets/PPC.h:448
+  void setMaxAtomicWidth() override {
+    // For layout on ELF targets, we support up to 16 bytes.
+    if (getTriple().isOSBinFormatELF())
----------------
hubert.reinterpretcast wrote:
> I believe this should be presented more as this override being implemented currently only for ELF targets. The current presentation seems to imply more design intent for non-ELF targets than there is consensus for.
> 
> For example:
> ```
> if (!getTriple().isOSBinFormatELF())
>   return PPCTargetInfo::setMaxAtomicWidth();
> ```
It looks a chance to adjust according to arch level to me(Considering we finally will make xcoff and elf targets consistent here). If you have strong opinion on this, I'll make a change here.


================
Comment at: clang/test/CodeGen/PowerPC/atomic-alignment.c:34
 
 // PPC32: @o = global %struct.O zeroinitializer, align 1{{$}}
 // PPC64: @o = global %struct.O zeroinitializer, align 8{{$}}
----------------
hubert.reinterpretcast wrote:
> Just noting that GCC increases the alignment even for ppc32:
> ```
> typedef struct A8 { char x[8]; } A8;
> typedef struct A16 { char x[16]; } A16;
> extern char q8[_Alignof(_Atomic(A8))], q8[8]; // okay for GCC targeting ppc32
> extern char q16[_Alignof(_Atomic(A16))], q16[16]; // okay for GCC targeting ppc32
> ```
> 
> Apparently, the change for i686 in GCC occurred with version 11.
> https://godbolt.org/z/fTTGoqWW1
I'll post another patch to address ppc32 issue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377