[Power] Improve the expansion of atomic loads/stores

[morisset]

Summary

Atomic loads and store of up to the native size (32 bits, or 64 for PPC64)
can be lowered to a simple load or store instruction (as the synchronization
is already handled by AtomicExpand, and the atomicity is guaranteed thanks to
the alignment requirements of atomic accesses). This is exactly what this patch
does. Previously, these were implemented by complex
load-linked/store-conditional loops.. an obvious performance problem.

For example, this patch turns

define void @store_i8_unordered(i8* %mem) {
  store atomic i8 42, i8* %mem unordered, align 1
  ret void
}

from

_store_i8_unordered:                    ; @store_i8_unordered
; BB#0:
    rlwinm r2, r3, 3, 27, 28
    li r4, 42
    xori r5, r2, 24
    rlwinm r2, r3, 0, 0, 29
    li r3, 255
    slw r4, r4, r5
    slw r3, r3, r5
    and r4, r4, r3
LBB4_1:                                 ; =>This Inner Loop Header: Depth=1
    lwarx r5, 0, r2
    andc r5, r5, r3
    or r5, r4, r5
    stwcx. r5, 0, r2
    bne cr0, LBB4_1
; BB#2:
    blr

into

_store_i8_unordered:                    ; @store_i8_unordered
; BB#0:
    li r2, 42
    stb r2, 0(r3)
    blr

which looks like a pretty clear win to me.

[hfinkel]

Is this something guaranteed by the ISA, or just something we know to be true for common implementations? I'm somewhat afraid of doing this for the generic targets in case this might not be true for some embedded implementation (if it is not an ISA guarantee).

[wschmidt]

I agree that this is legitimate according to the alignment definitions of atomic loads and stores and the atomicity requirements of the PowerPC ISA. A most excellent improvement indeed.

From the ISA:

Vector storage accesses are not guaranteed to be atomic. The following other types of single-register accesses are always atomic:

• byte accesses (all bytes are aligned on byte boundaries);
• halfword accesses aligned on halfword boundaries;
• word accesses aligned on word boundaries;
• doubleword accesses aligned on doubleword boundaries.

The language in the LLVM IR reference indicates that if this is not the case for load atomic or store atomic, the result is undefined. So this is safe.

[wschmidt]

The easiest way to force a use of the indexed forms of load and store is to use an offset that is out of range of the hardware load/store-immediate instructions. So try an array reference that is more than 65535 bytes away from its base.

[wschmidt]

(BTW, the doubleword accesses only apply to 64-bit -- I got lazy and didn't provide the parenthetical about that.)

[morisset]

Thank you ! This paragraph of the documentation was indeed what I was
thinking about, when saying that atomicity is not a problem, I should have
made it more explicit. I will try to generate tests for the indexed
versions and add them in.

[hfinkel]

LGTM.

Regarding the indexed loads/stores, as bill said, you should be able to generate them using a large offset (that won't fit in the immediate), or just using a variable offset.

[morisset]

Add tests for indexed loads/stores

[hfinkel]

Thanks! Commit whenever you're ready.

test/CodeGen/PowerPC/atomics-indexed.ll

2 ↗

(On Diff #14353)

If at some point you could look at fixing this, that would be great.

[morisset]

Closed by commit rL218922 (authored by @morisset).

lib/Target/PowerPC/PPCISelLowering.cpp

if (Subtarget.hasAltivec()) {
     }
   }
 
   if (Subtarget.has64BitSupport()) {
     setOperationAction(ISD::PREFETCH, MVT::Other, Legal);
     setOperationAction(ISD::READCYCLECOUNTER, MVT::i64, Legal);
   }
 
-  setOperationAction(ISD::ATOMIC_LOAD,  MVT::i32, Expand);
-  setOperationAction(ISD::ATOMIC_STORE, MVT::i32, Expand);
+  if (!isPPC64) {
     setOperationAction(ISD::ATOMIC_LOAD,  MVT::i64, Expand);
     setOperationAction(ISD::ATOMIC_STORE, MVT::i64, Expand);
+  }
 
   setBooleanContents(ZeroOrOneBooleanContent);
   // Altivec instructions set fields to all zeros or all ones.
   setBooleanVectorContents(ZeroOrNegativeOneBooleanContent);
 
   if (!isPPC64) {
     // These libcalls are not available in 32-bit.
     setLibcallName(RTLIB::SHL_I128, nullptr);

lib/Target/PowerPC/PPCInstr64Bit.td

 // addresses without at least 4-byte alignment.
 def : Pat<(i64 (unaligned4sextloadi32 xoaddr:$src)),
           (LWAX xoaddr:$src)>;
 def : Pat<(i64 (unaligned4load xoaddr:$src)),
           (LDX xoaddr:$src)>;
 def : Pat<(unaligned4store i64:$rS, xoaddr:$dst),
           (STDX $rS, xoaddr:$dst)>;
 
+// 64-bits atomic loads and stores
+def : Pat<(atomic_load_64 ixaddr:$src), (LD  memrix:$src)>;
+def : Pat<(atomic_load_64 xaddr:$src),  (LDX memrr:$src)>;
+
+def : Pat<(atomic_store_64 ixaddr:$ptr, i64:$val), (STD  g8rc:$val, memrix:$ptr)>;
+def : Pat<(atomic_store_64 xaddr:$ptr,  i64:$val), (STDX g8rc:$val, memrr:$ptr)>;

lib/Target/PowerPC/PPCInstrInfo.td

 defm : TrapExtendedMnemonic<"ng", 20>;
 defm : TrapExtendedMnemonic<"llt", 2>;
 defm : TrapExtendedMnemonic<"lle", 6>;
 defm : TrapExtendedMnemonic<"lge", 5>;
 defm : TrapExtendedMnemonic<"lgt", 1>;
 defm : TrapExtendedMnemonic<"lnl", 5>;
 defm : TrapExtendedMnemonic<"lng", 6>;
 defm : TrapExtendedMnemonic<"u", 31>;
+
+// Atomic loads
+def : Pat<(atomic_load_8  iaddr:$src), (LBZ  memri:$src)>;
+def : Pat<(atomic_load_16 iaddr:$src), (LHZ  memri:$src)>;
+def : Pat<(atomic_load_32 iaddr:$src), (LWZ  memri:$src)>;
+def : Pat<(atomic_load_8  xaddr:$src), (LBZX memrr:$src)>;
+def : Pat<(atomic_load_16 xaddr:$src), (LHZX memrr:$src)>;
+def : Pat<(atomic_load_32 xaddr:$src), (LWZX memrr:$src)>;
+
+// Atomic stores
+def : Pat<(atomic_store_8  iaddr:$ptr, i32:$val), (STB  gprc:$val, memri:$ptr)>;
+def : Pat<(atomic_store_16 iaddr:$ptr, i32:$val), (STH  gprc:$val, memri:$ptr)>;
+def : Pat<(atomic_store_32 iaddr:$ptr, i32:$val), (STW  gprc:$val, memri:$ptr)>;
+def : Pat<(atomic_store_8  xaddr:$ptr, i32:$val), (STBX gprc:$val, memrr:$ptr)>;
+def : Pat<(atomic_store_16 xaddr:$ptr, i32:$val), (STHX gprc:$val, memrr:$ptr)>;
+def : Pat<(atomic_store_32 xaddr:$ptr, i32:$val), (STWX gprc:$val, memrr:$ptr)>;

test/CodeGen/PowerPC/atomic-2.ll

 ; CHECK: stdcx.
   ret i64 %tmp
 }
 
 define void @atomic_store(i64* %mem, i64 %val) nounwind {
 entry:
 ; CHECK: @atomic_store
   store atomic i64 %val, i64* %mem release, align 64
-; CHECK: ldarx
-; CHECK: stdcx.
+; CHECK: sync 1
+; CHECK-NOT: stdcx
+; CHECK: std
   ret void
 }
 
 define i64 @atomic_load(i64* %mem) nounwind {
 entry:
 ; CHECK: @atomic_load
   %tmp = load atomic i64* %mem acquire, align 64
-; CHECK: ldarx
-; CHECK: stdcx.
-; CHECK: stdcx.
+; CHECK-NOT: ldarx
+; CHECK: ld
+; CHECK: sync 1
   ret i64 %tmp
 }
 

test/CodeGen/PowerPC/atomics.ll

 ; RUN: llc < %s -mtriple=powerpc-apple-darwin -march=ppc32 -verify-machineinstrs | FileCheck %s --check-prefix=CHECK --check-prefix=PPC32
 ; FIXME: -verify-machineinstrs currently fail on ppc64 (mismatched register/instruction).
 ; This is already checked for in Atomics-64.ll
 ; RUN: llc < %s -mtriple=powerpc-apple-darwin -march=ppc64 | FileCheck %s --check-prefix=CHECK --check-prefix=PPC64
 
 ; FIXME: we don't currently check for the operations themselves with CHECK-NEXT,
 ;   because they are implemented in a very messy way with lwarx/stwcx.
 ;   It should be fixed soon in another patch.
 
 ; We first check loads, for all sizes from i8 to i64.
 ; We also vary orderings to check for barriers.
 define i8 @load_i8_unordered(i8* %mem) {
 ; CHECK-LABEL: load_i8_unordered
+; CHECK: lbz
 ; CHECK-NOT: sync
   %val = load atomic i8* %mem unordered, align 1
   ret i8 %val
 }
 define i16 @load_i16_monotonic(i16* %mem) {
 ; CHECK-LABEL: load_i16_monotonic
+; CHECK: lhz
 ; CHECK-NOT: sync
   %val = load atomic i16* %mem monotonic, align 2
   ret i16 %val
 }
 define i32 @load_i32_acquire(i32* %mem) {
 ; CHECK-LABEL: load_i32_acquire
+; CHECK: lwz
   %val = load atomic i32* %mem acquire, align 4
 ; CHECK: sync 1
   ret i32 %val
 }
 define i64 @load_i64_seq_cst(i64* %mem) {
 ; CHECK-LABEL: load_i64_seq_cst
 ; CHECK: sync 0
+; PPC32: __sync_
+; PPC64-NOT: __sync_
+; PPC64: ld
   %val = load atomic i64* %mem seq_cst, align 8
 ; CHECK: sync 1
   ret i64 %val
 }
 
 ; Stores
 define void @store_i8_unordered(i8* %mem) {
 ; CHECK-LABEL: store_i8_unordered
 ; CHECK-NOT: sync
+; CHECK: stb
   store atomic i8 42, i8* %mem unordered, align 1
   ret void
 }
 define void @store_i16_monotonic(i16* %mem) {
 ; CHECK-LABEL: store_i16_monotonic
 ; CHECK-NOT: sync
+; CHECK: sth
   store atomic i16 42, i16* %mem monotonic, align 2
   ret void
 }
 define void @store_i32_release(i32* %mem) {
 ; CHECK-LABEL: store_i32_release
 ; CHECK: sync 1
+; CHECK: stw
   store atomic i32 42, i32* %mem release, align 4
   ret void
 }
 define void @store_i64_seq_cst(i64* %mem) {
 ; CHECK-LABEL: store_i64_seq_cst
 ; CHECK: sync 0
+; PPC32: __sync_
+; PPC64-NOT: __sync_
+; PPC64: std
   store atomic i64 42, i64* %mem seq_cst, align 8
   ret void
 }
 
 ; Atomic CmpXchg
 define i8 @cas_strong_i8_sc_sc(i8* %mem) {
 ; CHECK-LABEL: cas_strong_i8_sc_sc
 ; CHECK: sync 0

test/CodeGen/PowerPC/pr15630.ll

 ; RUN: llc -mcpu=pwr7 -O0 < %s | FileCheck %s
 
 target datalayout = "E-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v128:128:128-n32:64"
 target triple = "powerpc64-unknown-linux-gnu"
 
 define weak_odr void @_D4core6atomic49__T11atomicStoreVE4core6atomic11MemoryOrder3ThThZ11atomicStoreFNaNbKOhhZv(i8* %val_arg, i8 zeroext %newval_arg) {
 entry:
   %newval = alloca i8
   %ordering = alloca i32, align 4
   store i8 %newval_arg, i8* %newval
   %tmp = load i8* %newval
   store atomic volatile i8 %tmp, i8* %val_arg seq_cst, align 1
   ret void
 }
 
-; CHECK: stwcx.
+; CHECK: sync
+; CHECK: stb