[Power] Use AtomicExpandPass for fence insertion, and use lwsync where appropriate

[morisset]

Summary

This patch makes use of AtomicExpandPass in Power for inserting fences around
atomic as part of an effort to remove fence insertion from SelectionDAGBuilder.
As a big bonus, it lets us use sync 1 (lightweight sync, often used by the mnemonic
lwsync) instead of sync 0 (heavyweight sync) in many cases.

I also added a test, as there was no test for the barriers emitted by the Power
backend for atomic loads and stores.

[hfinkel]

lib/Target/PowerPC/PPCISelLowering.h

412 ↗

(On Diff #13234)

The indentation here seems odd.

414 ↗

(On Diff #13234)

Here too.

lib/Target/PowerPC/PPCInstrInfo.td

1710 ↗

(On Diff #13234)

This is correct, as such, but should really be enhanced. The PPC A2, for example, supports SYNC (even though it is a Book E processor), such that MSYNC is really just SYNC 0. Older Book E designs, like the PPC 440, only support MSYNC. We'll want to split SYNC support into its own feature.

lib/Target/PowerPC/PPCTargetMachine.cpp

105 ↗

(On Diff #13234)

Don't need a blank line here.

[morisset]

Fix formatting based on hfinkel comments.

About replacing the BookE flag by a special flag for the sync instruction,
should I try doing it in this patch, or is it best as a separate thing ?

[hfinkel]

• Original Message -----

From: "Robin Morisset" <[e-mail]>
To: [e-mail], [e-mail]
Cc: [e-mail], [e-mail]
Sent: Thursday, September 4, 2014 12:34:15 PM
Subject: Re: [PATCH] [Power] Use AtomicExpandPass for fence insertion, and use lwsync where appropriate

Fix formatting based on hfinkel comments.

About replacing the BookE flag by a special flag for the sync
instruction,
should I try doing it in this patch, or is it best as a separate
thing ?

Let's do that in a follow-up commit. Please add a FIXME in this patch so we remember to do it.

Thanks again,
Hal

http://reviews.llvm.org/D5180

Files:

include/llvm/IR/IntrinsicsPowerPC.td
lib/Target/PowerPC/PPCISelLowering.cpp
lib/Target/PowerPC/PPCISelLowering.h
lib/Target/PowerPC/PPCInstrInfo.td
lib/Target/PowerPC/PPCTargetMachine.cpp
test/CodeGen/PowerPC/atomics.ll

[morisset]

Add a Fixme about the use of IsBookE flag for sync support.

[wschmidt]

Minor typo on line 6558

lib/Target/PowerPC/PPCISelLowering.cpp

6558 ↗

(On Diff #13276)

dependant => dependent

[morisset]

Fix typo and add comment.

[wschmidt]

I've reviewed this against what gcc does, and your emitLeadingFence matches exactly. As you've noted, what you're doing for emitTrailingFence is conservative. GCC generates an isync in rs6000.c:rs6000_post_atomic_barrier() for these cases.

In short, the patch LGTM, and you can improve it later if desired.

[morisset]

Closed by commit rL218331 (authored by @morisset).

llvm/trunk/include/llvm/IR/IntrinsicsPowerPC.td

let TargetPrefix = "ppc" in {  // All intrinsics start with "llvm.ppc.".
   def int_ppc_dcbi  : Intrinsic<[], [llvm_ptr_ty], []>;
   def int_ppc_dcbst : Intrinsic<[], [llvm_ptr_ty], []>;
   def int_ppc_dcbt  : Intrinsic<[], [llvm_ptr_ty],
     [IntrReadWriteArgMem, NoCapture<0>]>;
   def int_ppc_dcbtst: Intrinsic<[], [llvm_ptr_ty], []>;
   def int_ppc_dcbz  : Intrinsic<[], [llvm_ptr_ty], []>;
   def int_ppc_dcbzl : Intrinsic<[], [llvm_ptr_ty], []>;
 
-  // sync instruction
+  // sync instruction (i.e. sync 0, a.k.a hwsync)
   def int_ppc_sync : Intrinsic<[], [], []>;
+  // lwsync is sync 1
+  def int_ppc_lwsync : Intrinsic<[], [], []>;
 
   // Intrinsics used to generate ctr-based loops. These should only be
   // generated by the PowerPC backend!
   def int_ppc_mtctr : Intrinsic<[], [llvm_anyint_ty], []>;
   def int_ppc_is_decremented_ctr_nonzero : Intrinsic<[llvm_i1_ty], [], []>;
 }
 
 

llvm/trunk/lib/Target/PowerPC/PPCISelLowering.h

public:
     unsigned getRegisterByName(const char* RegName, EVT VT) const override;
 
     void computeKnownBitsForTargetNode(const SDValue Op,
                                        APInt &KnownZero,
                                        APInt &KnownOne,
                                        const SelectionDAG &DAG,
                                        unsigned Depth = 0) const override;
 
+    Instruction* emitLeadingFence(IRBuilder<> &Builder, AtomicOrdering Ord,
+                                  bool IsStore, bool IsLoad) const override;
+    Instruction* emitTrailingFence(IRBuilder<> &Builder, AtomicOrdering Ord,
+                                   bool IsStore, bool IsLoad) const override;
+
     MachineBasicBlock *
       EmitInstrWithCustomInserter(MachineInstr *MI,
                                   MachineBasicBlock *MBB) const override;
     MachineBasicBlock *EmitAtomicBinary(MachineInstr *MI,
                                         MachineBasicBlock *MBB, bool is64Bit,
                                         unsigned BinOpcode) const;
     MachineBasicBlock *EmitPartwordAtomicBinary(MachineInstr *MI,
                                                 MachineBasicBlock *MBB,

llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp

void PPCTargetLowering::ReplaceNodeResults(SDNode *N,
   }
 }
 
 
 //===----------------------------------------------------------------------===//
 //  Other Lowering Code
 //===----------------------------------------------------------------------===//
 
+static Instruction* callIntrinsic(IRBuilder<> &Builder, Intrinsic::ID Id) {
+  Module *M = Builder.GetInsertBlock()->getParent()->getParent();
+  Function *Func = Intrinsic::getDeclaration(M, Id);
+  return Builder.CreateCall(Func);
+}
+
+// The mappings for emitLeading/TrailingFence is taken from
+// http://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html
+Instruction* PPCTargetLowering::emitLeadingFence(IRBuilder<> &Builder,
+                                         AtomicOrdering Ord, bool IsStore,
+                                         bool IsLoad) const {
+  if (Ord == SequentiallyConsistent)
+    return callIntrinsic(Builder, Intrinsic::ppc_sync);
+  else if (isAtLeastRelease(Ord))
+    return callIntrinsic(Builder, Intrinsic::ppc_lwsync);
+  else
+    return nullptr;
+}
+
+Instruction* PPCTargetLowering::emitTrailingFence(IRBuilder<> &Builder,
+                                          AtomicOrdering Ord, bool IsStore,
+                                          bool IsLoad) const {
+  if (IsLoad && isAtLeastAcquire(Ord))
+    return callIntrinsic(Builder, Intrinsic::ppc_lwsync);
+  // FIXME: this is too conservative, a dependent branch + isync is enough.
+  // See http://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html and
+  // http://www.rdrop.com/users/paulmck/scalability/paper/N2745r.2011.03.04a.html
+  // and http://www.cl.cam.ac.uk/~pes20/cppppc/ for justification.
+  else
+    return nullptr;
+}
+
 MachineBasicBlock *
 PPCTargetLowering::EmitAtomicBinary(MachineInstr *MI, MachineBasicBlock *BB,
                                     bool is64bit, unsigned BinOpcode) const {
   // This also handles ATOMIC_SWAP, indicated by BinOpcode==0.
   const TargetInstrInfo *TII =
       getTargetMachine().getSubtargetImpl()->getInstrInfo();
 
   const BasicBlock *LLVM_BB = BB->getBasicBlock();

llvm/trunk/lib/Target/PowerPC/PPCInstrInfo.td

 
 let isCodeGenOnly = 1 in {
   def MSYNC : XForm_24_sync<31, 598, (outs), (ins),
                            "msync", IIC_LdStSync, []>, Requires<[IsBookE]> {
     let L = 0;
   }
 }
 
-def : Pat<(int_ppc_sync), (SYNC 0)>, Requires<[IsNotBookE]>;
-def : Pat<(int_ppc_sync), (MSYNC)>, Requires<[IsBookE]>;
+// FIXME: We should use a specific flag to check for the presence of the sync
+// instruction instead of relying on IsBookE.
+// For example, the PPC A2 supports SYNC despite being a BookE processor.
+def : Pat<(int_ppc_sync),   (SYNC 0)>, Requires<[IsNotBookE]>;
+def : Pat<(int_ppc_lwsync), (SYNC 1)>, Requires<[IsNotBookE]>;
+def : Pat<(int_ppc_sync),   (MSYNC)>, Requires<[IsBookE]>;
+def : Pat<(int_ppc_lwsync), (MSYNC)>, Requires<[IsBookE]>;
 
 //===----------------------------------------------------------------------===//
 // PPC32 Arithmetic Instructions.
 //
 
 let PPC970_Unit = 1 in {  // FXU Operations.
 def ADDI   : DForm_2<14, (outs gprc:$rD), (ins gprc_nor0:$rA, s16imm:$imm),
                      "addi $rD, $rA, $imm", IIC_IntSimple,

llvm/trunk/lib/Target/PowerPC/PPCTargetMachine.cpp

public:
   PPCTargetMachine &getPPCTargetMachine() const {
     return getTM<PPCTargetMachine>();
   }
 
   const PPCSubtarget &getPPCSubtarget() const {
     return *getPPCTargetMachine().getSubtargetImpl();
   }
 
+  void addIRPasses() override;
   bool addPreISel() override;
   bool addILPOpts() override;
   bool addInstSelector() override;
   bool addPreRegAlloc() override;
   bool addPreSched2() override;
   bool addPreEmitPass() override;
 };
 } // namespace
 
 TargetPassConfig *PPCTargetMachine::createPassConfig(PassManagerBase &PM) {
   return new PPCPassConfig(this, PM);
 }
 
+void PPCPassConfig::addIRPasses() {
+  addPass(createAtomicExpandPass(&getPPCTargetMachine()));
+  TargetPassConfig::addIRPasses();
+}
+
 bool PPCPassConfig::addPreISel() {
   if (!DisableCTRLoops && getOptLevel() != CodeGenOpt::None)
     addPass(createPPCCTRLoops(getPPCTargetMachine()));
 
   return false;
 }
 
 bool PPCPassConfig::addILPOpts() {

llvm/trunk/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-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-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
+  %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
+  %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
+  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
+  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
+  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
+  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
+  %val = cmpxchg i8* %mem, i8 0, i8 1 seq_cst seq_cst
+; CHECK: sync 1
+  %loaded = extractvalue { i8, i1} %val, 0
+  ret i8 %loaded
+}
+define i16 @cas_weak_i16_acquire_acquire(i16* %mem) {
+; CHECK-LABEL: cas_weak_i16_acquire_acquire
+;CHECK-NOT: sync
+  %val = cmpxchg weak i16* %mem, i16 0, i16 1 acquire acquire
+; CHECK: sync 1
+  %loaded = extractvalue { i16, i1} %val, 0
+  ret i16 %loaded
+}
+define i32 @cas_strong_i32_acqrel_acquire(i32* %mem) {
+; CHECK-LABEL: cas_strong_i32_acqrel_acquire
+; CHECK: sync 1
+  %val = cmpxchg i32* %mem, i32 0, i32 1 acq_rel acquire
+; CHECK: sync 1
+  %loaded = extractvalue { i32, i1} %val, 0
+  ret i32 %loaded
+}
+define i64 @cas_weak_i64_release_monotonic(i64* %mem) {
+; CHECK-LABEL: cas_weak_i64_release_monotonic
+; CHECK: sync 1
+  %val = cmpxchg weak i64* %mem, i64 0, i64 1 release monotonic
+; CHECK-NOT: [sync ]
+  %loaded = extractvalue { i64, i1} %val, 0
+  ret i64 %loaded
+}
+
+; AtomicRMW
+define i8 @add_i8_monotonic(i8* %mem, i8 %operand) {
+; CHECK-LABEL: add_i8_monotonic
+; CHECK-NOT: sync
+  %val = atomicrmw add i8* %mem, i8 %operand monotonic
+  ret i8 %val
+}
+define i16 @xor_i16_seq_cst(i16* %mem, i16 %operand) {
+; CHECK-LABEL: xor_i16_seq_cst
+; CHECK: sync 0
+  %val = atomicrmw xor i16* %mem, i16 %operand seq_cst
+; CHECK: sync 1
+  ret i16 %val
+}
+define i32 @xchg_i32_acq_rel(i32* %mem, i32 %operand) {
+; CHECK-LABEL: xchg_i32_acq_rel
+; CHECK: sync 1
+  %val = atomicrmw xchg i32* %mem, i32 %operand acq_rel
+; CHECK: sync 1
+  ret i32 %val
+}
+define i64 @and_i64_release(i64* %mem, i64 %operand) {
+; CHECK-LABEL: and_i64_release
+; CHECK: sync 1
+  %val = atomicrmw and i64* %mem, i64 %operand release
+; CHECK-NOT: [sync ]
+  ret i64 %val
+}