[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

12879

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

12892

Same here - ARMv7-AR?

[weimingz]

lib/Target/ARM/ARMISelLowering.cpp

12879

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

[jmolloy]

Ah, sorry, I'd missed that!

LGTM, cheers!

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 (Subtarget->isARMv6m())
+      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))
+  return (Size <= (Subtarget->isMClass() ? 32U : 64U) && !Subtarget->isARMv6m())
              ? 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;
+  return getTargetMachine().getOptLevel() != 0 && !Subtarget->isARMv6m();
 }
 
 bool ARMTargetLowering::shouldInsertFencesForAtomic(
     const Instruction *I) const {
   return InsertFencesForAtomic;
 }
 
 // This has so far only been implemented for MachO.
 bool ARMTargetLowering::useLoadStackGuardNode() const {

lib/Target/ARM/ARMSubtarget.h

public:
   bool isCortexA8() const { return ARMProcFamily == CortexA8; }
   bool isCortexA9() const { return ARMProcFamily == CortexA9; }
   bool isCortexA15() const { return ARMProcFamily == CortexA15; }
   bool isSwift()    const { return ARMProcFamily == Swift; }
   bool isCortexM3() const { return ARMProcFamily == CortexM3; }
   bool isLikeA9() const { return isCortexA9() || isCortexA15() || isKrait(); }
   bool isCortexR5() const { return ARMProcFamily == CortexR5; }
   bool isKrait() const { return ARMProcFamily == Krait; }
+  bool isARMv6m() const { return ARMArch == ARMv6m; }
   /// @}
 
   bool hasARMOps() const { return !NoARM; }
 
   bool hasVFP2() const { return HasVFPv2; }
   bool hasVFP3() const { return HasVFPv3; }
   bool hasVFP4() const { return HasVFPv4; }
   bool hasFPARMv8() const { return HasFPARMv8; }

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());
+  return hasAnyDataBarrier() &&
+         (!isThumb() || hasV8MBaselineOps() || ARMArch == ARMv6m);
 }
 
 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());
 }

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]
; 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
 }