[Cortex-M0] Atomic lowering

[weimingz]

Summary

ARMv6m supports dmb etc fench instructions but not ldrex/strex etc. So for some atomic load/store, LLVM should inline instructions instead of lowering to __sync_ calls.

[efriedma]

Explicitly checking isARMv6m() is wrong... we want to check features, not specific architectures. I think here, we're interested here in the behavior of targets with the following characteristics:

(1) Are in thumb mode (isThumb())
(2) Support a memory barrier instruction (hasAnyDataBarrier())
(3) Don't support ldrex/strex (!hasV8MBaselineOps())

I think ARMv6sm also has these characteristics; checking features makes the code more clear, and avoids mistakes.

---

It would also be a good idea to add tests to check that this patch doesn't change the generated code for other atomic operations.

[weimingz]

Hi Eli,

I think the problem is the feature for armv6m is not well defined.
ARMv6m is mostly Thumb1, but it also supports 5 32-bit instrs: DL, DMB, DSB. ISM. MRS, MSR

In ARM.td, FeatureDB has already been defined for Armv6m.
However, when existing code actually checks for availability of atomic feature (fence + ldrex/strex), if the target is Thumb but not supports ARMv8mBaseline, then it assumes no such feature.

For example: in ARMISelLowering.cpp::1043

if (Subtarget->hasAnyDataBarrier() &&
    (!Subtarget->isThumb() || Subtarget->hasV8MBaselineOps())

[efriedma]

bool ARMSubtarget::enableAtomicExpand() const {
  return hasAnyDataBarrier();
}

TargetLowering::AtomicExpansionKind
ARMTargetLowering::shouldExpandAtomicRMWInIR(AtomicRMWInst *AI) const {
  unsigned Size = AI->getType()->getPrimitiveSizeInBits();
  return ((!isThumb() || hasV8MBaselineOps()) && Size <= (Subtarget->isMClass() ? 32U : 64U))
             ? AtomicExpansionKind::LLSC
             : AtomicExpansionKind::None;
}

Is this not sufficient for some reason?

[weimingz]

Agree with the change in shouldExpandAtomicRMWInIR but for ARMSubtarget::enableAtomicExpand(),
changing from

return hasAnyDataBarrier() && (!isThumb() || hasV8MBaselineOps()

to

return hasAnyDataBarrier();

Is it too relax? Seems OK but not very sure.

[efriedma]

Should be fine; the whole point of this patch is that you want to run atomic expansion for targets which have dmb, but not ldrex/strex.

[weimingz]

Fix the test case and remove isARMv6m()

[efriedma]

Looks fine, but someone more familiar with the ARM backend should double-check I didn't miss anything.

[jmolloy]

lib/Target/ARM/ARMISelLowering.cpp

12880 ↗

(On Diff #76500)

What about ARMv7-A/R here? Previously we'd return LLSC in this case.

12894 ↗

(On Diff #76500)

Same here - ARMv7-AR?

[weimingz]

lib/Target/ARM/ARMISelLowering.cpp

12880 ↗

(On Diff #76500)

ARMv7-A/R should not be affected because they have V8MBaselineOps() (inherits from v6t2)

[jmolloy]

Ah, sorry, I'd missed that!

LGTM, cheers!

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

if (Subtarget->hasAnyDataBarrier() &&
     // if they can be combined with nearby atomic loads and stores.
     if (!Subtarget->hasV8Ops() || getTargetMachine().getOptLevel() == 0) {
       // Automatically insert fences (dmb ish) around ATOMIC_SWAP etc.
       InsertFencesForAtomic = true;
     }
   } else {
     // If there's anything we can use as a barrier, go through custom lowering
     // for ATOMIC_FENCE.
+    // If target has DMB in thumb, Fences can be inserted.
+    if (Subtarget->hasDataBarrier())
+      InsertFencesForAtomic = true;
+
     setOperationAction(ISD::ATOMIC_FENCE,   MVT::Other,
                        Subtarget->hasAnyDataBarrier() ? Custom : Expand);
 
     // Set them all for expansion, which will force libcalls.
     setOperationAction(ISD::ATOMIC_CMP_SWAP,  MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_SWAP,      MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_LOAD_ADD,  MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_LOAD_SUB,  MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_LOAD_AND,  MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_LOAD_OR,   MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_LOAD_XOR,  MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_LOAD_NAND, MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_LOAD_MIN, MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_LOAD_MAX, MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_LOAD_UMIN, MVT::i32, Expand);
     setOperationAction(ISD::ATOMIC_LOAD_UMAX, MVT::i32, Expand);
     // Mark ATOMIC_LOAD and ATOMIC_STORE custom so we can handle the
     // Unordered/Monotonic case.
+    if (!InsertFencesForAtomic) {
       setOperationAction(ISD::ATOMIC_LOAD, MVT::i32, Custom);
       setOperationAction(ISD::ATOMIC_STORE, MVT::i32, Custom);
     }
+  }
 
   setOperationAction(ISD::PREFETCH,         MVT::Other, Custom);
 
   // Requires SXTB/SXTH, available on v6 and up in both ARM and Thumb modes.
   if (!Subtarget->hasV6Ops()) {
     setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i16, Expand);
     setOperationAction(ISD::SIGN_EXTEND_INREG, MVT::i8,  Expand);
   }
return ((Size == 64) && !Subtarget->isMClass()) ? AtomicExpansionKind::LLOnly
                                                   : AtomicExpansionKind::None;
 }
 
 // For the real atomic operations, we have ldrex/strex up to 32 bits,
 // and up to 64 bits on the non-M profiles
 TargetLowering::AtomicExpansionKind
 ARMTargetLowering::shouldExpandAtomicRMWInIR(AtomicRMWInst *AI) const {
   unsigned Size = AI->getType()->getPrimitiveSizeInBits();
-  return (Size <= (Subtarget->isMClass() ? 32U : 64U))
+  bool hasAtomicRMW = !Subtarget->isThumb() || Subtarget->hasV8MBaselineOps();
+  return (Size <= (Subtarget->isMClass() ? 32U : 64U) && hasAtomicRMW)
              ? AtomicExpansionKind::LLSC
              : AtomicExpansionKind::None;
 }
 
 bool ARMTargetLowering::shouldExpandAtomicCmpXchgInIR(
     AtomicCmpXchgInst *AI) const {
   // At -O0, fast-regalloc cannot cope with the live vregs necessary to
   // implement cmpxchg without spilling. If the address being exchanged is also
   // on the stack and close enough to the spill slot, this can lead to a
   // situation where the monitor always gets cleared and the atomic operation
   // can never succeed. So at -O0 we need a late-expanded pseudo-inst instead.
-  return getTargetMachine().getOptLevel() != 0;
+  bool hasAtomicCmpXchg =
+      !Subtarget->isThumb() || Subtarget->hasV8MBaselineOps();
+  return getTargetMachine().getOptLevel() != 0 && hasAtomicCmpXchg;
 }
 
 bool ARMTargetLowering::shouldInsertFencesForAtomic(
     const Instruction *I) const {
   return InsertFencesForAtomic;
 }
 
 // This has so far only been implemented for MachO.

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

 bool ARMSubtarget::enablePostRAScheduler() const {
   // No need for PostRA scheduling on out of order CPUs (for now restricted to
   // swift).
   if (getSchedModel().isOutOfOrder() && isSwift())
     return false;
   return (!isThumb() || hasThumb2());
 }
 
-bool ARMSubtarget::enableAtomicExpand() const {
-  return hasAnyDataBarrier() && (!isThumb() || hasV8MBaselineOps());
-}
+bool ARMSubtarget::enableAtomicExpand() const { return hasAnyDataBarrier(); }
 
 bool ARMSubtarget::useStride4VFPs(const MachineFunction &MF) const {
   // For general targets, the prologue can grow when VFPs are allocated with
   // stride 4 (more vpush instructions). But WatchOS uses a compact unwind
   // format which it's more important to get right.
   return isTargetWatchABI() || (isSwift() && !MF.getFunction()->optForMinSize());
 }
 

llvm/trunk/test/CodeGen/ARM/atomic-op.ll

 ; RUN: llc < %s -mtriple=armv7-apple-ios -verify-machineinstrs | FileCheck %s --check-prefix=CHECK --check-prefix CHECK-ARMV7
 ; RUN: llc < %s -mtriple=thumbv7-apple-ios -verify-machineinstrs | FileCheck %s --check-prefix=CHECK --check-prefix=CHECK-T2
 ; RUN: llc < %s -mtriple=thumbv6-apple-ios -verify-machineinstrs | FileCheck %s --check-prefix=CHECK-T1
-; RUN: llc < %s -mtriple=thumbv6-apple-ios -verify-machineinstrs -mcpu=cortex-m0 | FileCheck %s --check-prefix=CHECK-T1
+; RUN: llc < %s -mtriple=thumbv6-apple-ios -verify-machineinstrs -mcpu=cortex-m0 | FileCheck %s --check-prefix=CHECK-T1-M0
 ; RUN: llc < %s -mtriple=thumbv7--none-eabi -thread-model single -verify-machineinstrs | FileCheck %s --check-prefix=CHECK-BAREMETAL
 
 target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
 
 define void @func(i32 %argc, i8** %argv) nounwind {
 entry:
   %argc.addr = alloca i32   ; <i32*> [#uses=1]
   %argv.addr = alloca i8**    ; <i8***> [#uses=1]
entry:
   store i32 3855, i32* %ort
   store i32 3855, i32* %xort
   store i32 4, i32* %temp
   %tmp = load i32, i32* %temp
   ; CHECK: ldrex
   ; CHECK: add
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_add_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_add_4
   ; CHECK-BAREMETAL: add
   ; CHECK-BAREMETAL-NOT: __sync
   %0 = atomicrmw add i32* %val1, i32 %tmp monotonic
   store i32 %0, i32* %old
   ; CHECK: ldrex
   ; CHECK: sub
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_sub_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_sub_4
   ; CHECK-BAREMETAL: sub
   ; CHECK-BAREMETAL-NOT: __sync
   %1 = atomicrmw sub i32* %val2, i32 30 monotonic
   store i32 %1, i32* %old
   ; CHECK: ldrex
   ; CHECK: add
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_add_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_add_4
   ; CHECK-BAREMETAL: add
   ; CHECK-BAREMETAL-NOT: __sync
   %2 = atomicrmw add i32* %val2, i32 1 monotonic
   store i32 %2, i32* %old
   ; CHECK: ldrex
   ; CHECK: sub
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_sub_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_sub_4
   ; CHECK-BAREMETAL: sub
   ; CHECK-BAREMETAL-NOT: __sync
   %3 = atomicrmw sub i32* %val2, i32 1 monotonic
   store i32 %3, i32* %old
   ; CHECK: ldrex
   ; CHECK: and
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_and_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_and_4
   ; CHECK-BAREMETAL: and
   ; CHECK-BAREMETAL-NOT: __sync
   %4 = atomicrmw and i32* %andt, i32 4080 monotonic
   store i32 %4, i32* %old
   ; CHECK: ldrex
   ; CHECK: or
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_or_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_or_4
   ; CHECK-BAREMETAL: or
   ; CHECK-BAREMETAL-NOT: __sync
   %5 = atomicrmw or i32* %ort, i32 4080 monotonic
   store i32 %5, i32* %old
   ; CHECK: ldrex
   ; CHECK: eor
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_xor_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_xor_4
   ; CHECK-BAREMETAL: eor
   ; CHECK-BAREMETAL-NOT: __sync
   %6 = atomicrmw xor i32* %xort, i32 4080 monotonic
   store i32 %6, i32* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_min_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_min_4
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %7 = atomicrmw min i32* %val2, i32 16 monotonic
   store i32 %7, i32* %old
   %neg = sub i32 0, 1
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_min_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_min_4
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %8 = atomicrmw min i32* %val2, i32 %neg monotonic
   store i32 %8, i32* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_max_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_max_4
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %9 = atomicrmw max i32* %val2, i32 1 monotonic
   store i32 %9, i32* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_max_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_max_4
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %10 = atomicrmw max i32* %val2, i32 0 monotonic
   store i32 %10, i32* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umin_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umin_4
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %11 = atomicrmw umin i32* %val2, i32 16 monotonic
   store i32 %11, i32* %old
   %uneg = sub i32 0, 1
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umin_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umin_4
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %12 = atomicrmw umin i32* %val2, i32 %uneg monotonic
   store i32 %12, i32* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umax_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umax_4
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %13 = atomicrmw umax i32* %val2, i32 1 monotonic
   store i32 %13, i32* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umax_4
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umax_4
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %14 = atomicrmw umax i32* %val2, i32 0 monotonic
   store i32 %14, i32* %old
 
   ret void
 }
 
 define void @func2() nounwind {
 entry:
   %val = alloca i16
   %old = alloca i16
   store i16 31, i16* %val
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umin_2
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umin_2
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %0 = atomicrmw umin i16* %val, i16 16 monotonic
   store i16 %0, i16* %old
   %uneg = sub i16 0, 1
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umin_2
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umin_2
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %1 = atomicrmw umin i16* %val, i16 %uneg monotonic
   store i16 %1, i16* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umax_2
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umax_2
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %2 = atomicrmw umax i16* %val, i16 1 monotonic
   store i16 %2, i16* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umax_2
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umax_2
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %3 = atomicrmw umax i16* %val, i16 0 monotonic
   store i16 %3, i16* %old
   ret void
 }
 
 define void @func3() nounwind {
 entry:
   %val = alloca i8
   %old = alloca i8
   store i8 31, i8* %val
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umin_1
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umin_1
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %0 = atomicrmw umin i8* %val, i8 16 monotonic
   store i8 %0, i8* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umin_1
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umin_1
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %uneg = sub i8 0, 1
   %1 = atomicrmw umin i8* %val, i8 %uneg monotonic
   store i8 %1, i8* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umax_1
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umax_1
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %2 = atomicrmw umax i8* %val, i8 1 monotonic
   store i8 %2, i8* %old
   ; CHECK: ldrex
   ; CHECK: cmp
   ; CHECK: strex
   ; CHECK-T1: bl ___sync_fetch_and_umax_1
+  ; CHECK-T1-M0: bl ___sync_fetch_and_umax_1
   ; CHECK-BAREMETAL: cmp
   ; CHECK-BAREMETAL-NOT: __sync
   %3 = atomicrmw umax i8* %val, i8 0 monotonic
   store i8 %3, i8* %old
   ret void
 }
 
 ; CHECK: func4
; CHECK-LABEL: load_load_add_acquire
   %tmp = add i32 %val1, %val2
 
 ; CHECK: ldr {{r[0-9]}}, [r0]
 ; CHECK: dmb
 ; CHECK: ldr {{r[0-9]}}, [r1]
 ; CHECK: dmb
 ; CHECK: add r0,
 
+; CHECK-T1-M0: ldr {{r[0-9]}}, [r0]
+; CHECK-T1-M0: dmb
+; CHECK-T1-M0: ldr {{r[0-9]}}, [r1]
+; CHECK-T1-M0: dmb
+
 ; CHECK-T1: ___sync_val_compare_and_swap_4
 ; CHECK-T1: ___sync_val_compare_and_swap_4
 
 ; CHECK-BAREMETAL: ldr {{r[0-9]}}, [r0]
 ; CHECK-BAREMETAL-NOT: dmb
 ; CHECK-BAREMETAL: ldr {{r[0-9]}}, [r1]
 ; CHECK-BAREMETAL-NOT: dmb
 ; CHECK-BAREMETAL: add r0,

 ; CHECK: dmb
 ; CHECK: str r1, [r0]
 ; CHECK: dmb
 ; CHECK: str r3, [r2]
 
 ; CHECK-T1: ___sync_lock_test_and_set
 ; CHECK-T1: ___sync_lock_test_and_set
 
+; CHECK-T1-M0: dmb
+; CHECK-T1-M0: str r1, [r0]
+; CHECK-T1-M0: dmb
+; CHECK-T1-M0: str r3, [r2]
+
 ; CHECK-BAREMETAL-NOT: dmb
 ; CHECK-BAREMTEAL: str r1, [r0]
 ; CHECK-BAREMETAL-NOT: dmb
 ; CHECK-BAREMTEAL: str r3, [r2]
 
   ret void
 }
 
 define void @load_fence_store_monotonic(i32* %mem1, i32* %mem2) {
 ; CHECK-LABEL: load_fence_store_monotonic
   %val = load atomic i32, i32* %mem1 monotonic, align 4
   fence seq_cst
   store atomic i32 %val, i32* %mem2 monotonic, align 4
 
 ; CHECK: ldr [[R0:r[0-9]]], [r0]
 ; CHECK: dmb
 ; CHECK: str [[R0]], [r1]
 
+; CHECK-T1-M0: ldr [[R0:r[0-9]]], [r0]
+; CHECK-T1-M0: dmb
+; CHECK-T1-M0: str [[R0]], [r1]
+
 ; CHECK-T1: ldr [[R0:r[0-9]]], [{{r[0-9]+}}]
 ; CHECK-T1: {{dmb|bl ___sync_synchronize}}
 ; CHECK-T1: str [[R0]], [{{r[0-9]+}}]
 
 ; CHECK-BAREMETAL: ldr [[R0:r[0-9]]], [r0]
 ; CHECK-BAREMETAL-NOT: dmb
 ; CHECK-BAREMETAL: str [[R0]], [r1]
 
   ret void
 }