[ARM, Fix] Fix emitLeading/TrailingFence on old ARM processors

[morisset]

Summary

I had only tested this code for ARMv7 and ARMv8. This patch adds several
fallback paths if the processor does not support dmb ish:

• dmb sy if a cortex-M with support for dmb
• mcr p15, #0, r0, c7, c10, #5 for ARMv6 (special instruction equivalent to a DMB)
• call to __sync_synchronize otherwise.

These fallback paths were chosen based on the code for fence seq_cst.

Thanks to luqmana for having noticed this bug.

[luqmana]

Looks fine to me but I'm not really all that familiar with this code.

[jfb]

lib/Target/ARM/ARMISelLowering.cpp

11003 ↗

(On Diff #13568)

Is there a test for this call? We need to make sure it actually calls what you expect.

[morisset]

Thanks jf for catching this forgotten test!

It turned out that this code is dead, as these subtargets completely avoid this
code path anyway. I have replaced it by llvm_unreachable, and it is tested by
the ARMv4 test in atomic-load-store.

[jfb]

In D5304#7, @morisset wrote:

Thanks jf for catching this forgotten test!

It turned out that this code is dead, as these subtargets completely avoid this
code path anyway. I have replaced it by llvm_unreachable, and it is tested by
the ARMv4 test in atomic-load-store.

Is there a test that demonstrates the result of the other code path? I can't find code that generates the barrier in the tests, so it would be nice to add (maybe in a different change).

[morisset]

There is no barrier generated by the other path, the entire operation (store/load/rmw/whatever) is turned into a libcall on these old processors. It is tested (summarily) at the end of atomic-load-store.ll at least.

[morisset]

Rebase on trunk.

[jfb]

In D5304#10, @morisset wrote:

There is no barrier generated by the other path, the entire operation (store/load/rmw/whatever) is turned into a libcall on these old processors. It is tested (summarily) at the end of atomic-load-store.ll at least.

Sounds good, lgtm.

[morisset]

Closed by commit rL217965 (authored by @morisset).

llvm/trunk/lib/Target/ARM/ARMISelLowering.h

bool shouldConvertConstantLoadToIntImm(const APInt &Imm,
                                            Type *Ty) const override;
 
     /// \brief Returns true if an argument of type Ty needs to be passed in a
     /// contiguous block of registers in calling convention CallConv.
     bool functionArgumentNeedsConsecutiveRegisters(
         Type *Ty, CallingConv::ID CallConv, bool isVarArg) const override;
 
     bool hasLoadLinkedStoreConditional() const override;
+    Instruction *makeDMB(IRBuilder<> &Builder, ARM_MB::MemBOpt Domain) const;
     Value *emitLoadLinked(IRBuilder<> &Builder, Value *Addr,
                           AtomicOrdering Ord) const override;
     Value *emitStoreConditional(IRBuilder<> &Builder, Value *Val,
                                 Value *Addr, AtomicOrdering Ord) const override;
 
     void emitLeadingFence(IRBuilder<> &Builder, AtomicOrdering Ord,
                           bool IsStore, bool IsLoad) const override;
     void emitTrailingFence(IRBuilder<> &Builder, AtomicOrdering Ord,

llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp

bool ARMTargetLowering::shouldConvertConstantLoadToIntImm(const APInt &Imm,
   unsigned Bits = Ty->getPrimitiveSizeInBits();
   if (Bits == 0 || Bits > 32)
     return false;
   return true;
 }
 
 bool ARMTargetLowering::hasLoadLinkedStoreConditional() const { return true; }
 
-static void makeDMB(IRBuilder<> &Builder, ARM_MB::MemBOpt Domain) {
+Instruction* ARMTargetLowering::makeDMB(IRBuilder<> &Builder,
+                                        ARM_MB::MemBOpt Domain) const {
   Module *M = Builder.GetInsertBlock()->getParent()->getParent();
+
+  // First, if the target has no DMB, see what fallback we can use.
+  if (!Subtarget->hasDataBarrier()) {
+    // Some ARMv6 cpus can support data barriers with an mcr instruction.
+    // Thumb1 and pre-v6 ARM mode use a libcall instead and should never get
+    // here.
+    if (Subtarget->hasV6Ops() && !Subtarget->isThumb()) {
+      Function *MCR = llvm::Intrinsic::getDeclaration(M, Intrinsic::arm_mcr);
+      ArrayRef<Value*> args = {Builder.getInt32(15), Builder.getInt32(0),
+                               Builder.getInt32(0), Builder.getInt32(7),
+                               Builder.getInt32(10), Builder.getInt32(5)};
+      return Builder.CreateCall(MCR, args);
+    } else {
+      // Instead of using barriers, atomic accesses on these subtargets use
+      // libcalls.
+      llvm_unreachable("makeDMB on a target so old that it has no barriers");
+    }
+  } else {
     Function *DMB = llvm::Intrinsic::getDeclaration(M, Intrinsic::arm_dmb);
+    // Only a full system barrier exists in the M-class architectures.
+    Domain = Subtarget->isMClass() ? ARM_MB::SY : Domain;
     Constant *CDomain = Builder.getInt32(Domain);
-  Builder.CreateCall(DMB, CDomain);
+    return Builder.CreateCall(DMB, CDomain);
+  }
 }
 
 // Based on http://www.cl.cam.ac.uk/~pes20/cpp/cpp0xmappings.html
 void ARMTargetLowering::emitLeadingFence(IRBuilder<> &Builder,
                                          AtomicOrdering Ord, bool IsStore,
                                          bool IsLoad) const {
   if (!getInsertFencesForAtomic())
     return;

llvm/trunk/test/CodeGen/ARM/atomic-load-store.ll

 ; RUN: llc < %s -mtriple=armv7-apple-ios -verify-machineinstrs | FileCheck %s -check-prefix=ARM
 ; RUN: llc < %s -mtriple=armv7-apple-ios -O0 | FileCheck %s -check-prefix=ARM
 ; RUN: llc < %s -mtriple=thumbv7-apple-ios -verify-machineinstrs | FileCheck %s -check-prefix=THUMBTWO
 ; RUN: llc < %s -mtriple=thumbv6-apple-ios | FileCheck %s -check-prefix=THUMBONE
 ; RUN: llc < %s -mtriple=armv4-apple-ios | FileCheck %s -check-prefix=ARMV4
+; RUN: llc < %s -mtriple=armv6-apple-ios | FileCheck %s -check-prefix=ARMV6
+; RUN: llc < %s -mtriple=thumbv7m-apple-ios | FileCheck %s -check-prefix=THUMBM
 
 define void @test1(i32* %ptr, i32 %val1) {
 ; ARM-LABEL: test1
 ; ARM: dmb {{ish$}}
 ; ARM-NEXT: str
 ; ARM-NEXT: dmb {{ish$}}
 ; THUMBONE-LABEL: test1
 ; THUMBONE: __sync_lock_test_and_set_4
 ; THUMBTWO-LABEL: test1
 ; THUMBTWO: dmb {{ish$}}
 ; THUMBTWO-NEXT: str
 ; THUMBTWO-NEXT: dmb {{ish$}}
+; ARMV6-LABEL: test1
+; ARMV6: mcr p15, #0, {{r[0-9]*}}, c7, c10, #5
+; ARMV6: str
+; ARMV6: mcr p15, #0, {{r[0-9]*}}, c7, c10, #5
+; THUMBM-LABEL: test1
+; THUMBM: dmb sy
+; THUMBM: str
+; THUMBM: dmb sy
   store atomic i32 %val1, i32* %ptr seq_cst, align 4
   ret void
 }
 
 define i32 @test2(i32* %ptr) {
 ; ARM-LABEL: test2
 ; ARM: ldr
 ; ARM-NEXT: dmb {{ish$}}
 ; THUMBONE-LABEL: test2
 ; THUMBONE: __sync_val_compare_and_swap_4
 ; THUMBTWO-LABEL: test2
 ; THUMBTWO: ldr
 ; THUMBTWO-NEXT: dmb {{ish$}}
+; ARMV6-LABEL: test2
+; ARMV6: ldr
+; ARMV6: mcr p15, #0, {{r[0-9]*}}, c7, c10, #5
+; THUMBM-LABEL: test2
+; THUMBM: ldr
+; THUMBM: dmb sy
   %val = load atomic i32* %ptr seq_cst, align 4
   ret i32 %val
 }
 
 define void @test3(i8* %ptr1, i8* %ptr2) {
 ; ARM-LABEL: test3
 ; ARM-NOT: dmb
 ; ARM: ldrb

 ; THUMBTWO: bx lr
 
 ; THUMBONE-LABEL: test3
 ; THUMBONE-NOT: dmb
 ; THUMBONE: ldrb
 ; THUMBONE-NOT: dmb
 ; THUMBONE: strb
 ; THUMBONE-NOT: dmb
+
+; ARMV6-LABEL: test3
+; ARMV6-NOT: mcr
+; THUMBM-LABEL: test3
+; THUMBM-NOT: dmb sy
   %val = load atomic i8* %ptr1 unordered, align 1
   store atomic i8 %val, i8* %ptr2 unordered, align 1
   ret void
 }
 
 define void @test4(i8* %ptr1, i8* %ptr2) {
 ; THUMBONE-LABEL: test4
 ; THUMBONE: ___sync_val_compare_and_swap_1
 ; THUMBONE: ___sync_lock_test_and_set_1
+; ARMV6-LABEL: test4
+; THUMBM-LABEL: test4
   %val = load atomic i8* %ptr1 seq_cst, align 1
   store atomic i8 %val, i8* %ptr2 seq_cst, align 1
   ret void
 }
 
 define i64 @test_old_load_64bit(i64* %p) {
 ; ARMV4-LABEL: test_old_load_64bit
 ; ARMV4: ___sync_val_compare_and_swap_8