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

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: clang/test/CodeGen/PowerPC/atomic-alignment.c:14
 // PPC: @c = global i8 0, align 1{{$}}
 _Atomic(char) c; // expected-no-diagnostics
 
----------------
I was recently informed that, instead of using `expected-no-diagnostics` and `-verify`, using `-Werror` achieves the same with less processing cost. Sorry for the churn.

Noting also that `expected-no-diagnostics` only needs to be added once the the file.


================
Comment at: clang/test/CodeGen/PowerPC/quadword-atomics.c:5
+// RUN:   -emit-llvm -o - %s | FileCheck %s --check-prefix=PPC64
+
+typedef struct {
----------------
Can we add an AIX run line for `pwr7`? It should match the `PPC64` check lines.


================
Comment at: clang/test/CodeGen/PowerPC/quadword-atomics.c:42-44
+void test_add(int128_t *ptr, int128_t x) {
+  // expected-no-diagnostics
+  __c11_atomic_fetch_add((_Atomic(int128_t) *)ptr, x, __ATOMIC_RELAXED);
----------------
The pointer cast is avoidable; let's avoid it.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:18031
+bool PPCTargetLowering::shouldInlineQuadwordAtomics() const {
+  // TODO: 16-byte atomic type support for AIX is on progress, we should be able
+  // inline 16-byte atomic ops on AIX too in the future.
----------------
Typo fix; grammar fix.


================
Comment at: llvm/test/CodeGen/PowerPC/atomics-i128.ll:17
+; RUN:   --check-prefix=AIX64-PWR8 %s
+; RUN: llc -verify-machineinstrs -mtriple=powerpc-unknown-unknown -mcpu=pwr8 \
+; RUN:   -ppc-quadword-atomics -ppc-asm-full-reg-names -ppc-track-subreg-liveness < %s \
----------------
I think this case deserves an extra comment to explain the `-ppc-quadword-atomics`.


================
Comment at: llvm/test/CodeGen/PowerPC/atomics-i128.ll:77
+; AIX64-PWR8-NEXT:    sync
+; AIX64-PWR8-NEXT:    bl .__sync_lock_test_and_set_16[PR]
+; AIX64-PWR8-NEXT:    nop
----------------
What library is this expected to provide this symbol?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122377