Make check for atomic alignment target dependent

[vchuravy]

Summary

The goal is to relax the alignment check for the X86 architecure
restoring the behaviour exhibited by LLVM 3.8 and GCC.

According to the Intel Architectures Software Developer’s Manual, Volume 3A:

The integrity of a bus lock is not affected by the alignment of
the memory field. The LOCK semantics are followed for as many bus cycles
as necessary to update the entire operand. However, it is recommend that
locked accesses be aligned on their natural boundaries for better
system performance

also see http://joeduffyblog.com/2006/12/06/clr-data-alignment-and-cmpxchg8b/

Atomics operations like CMPXCHG16B for i128 do need to be naturally aligned.

Patch by: Valentin Churavy

[yuyichao]

Ref https://gcc.gnu.org/ml/gcc/2016-11/txt6ZlA_JS27i.txt .

While the instruction may guarantee atomicity without requiring alignment, I think there's still question whether the platform ABI (i.e. libatomic ABI linked above) allows it. I did not find any clear mention of the inlineability of unaligned i64 atomic on 32bit x86 but it does include other examples where the atomic is not inlineable when it's not properly aligned (e.g. in the _Atomic struct section).

Given that the draft is more or less a documentation and refinement of the current libatomic + clang/gcc behavior, I just checked what does it do on 32bit x86 for a 4bytes aligned i64 and it seems to be using the locking path. Unless that is changed and required, inlining 4bytes aligned i64 atomic will likely violate the libatomic ABI.

include/llvm/Target/TargetLowering.h

public:
   /// @}
 
   //===--------------------------------------------------------------------===//
   /// \name Helpers for atomic expansion.
   /// @{
 
   /// Returns the maximum atomic operation size (in bits) supported by
   /// the backend. Atomic operations greater than this size (as well
-  /// as ones that are not naturally aligned), will be expanded by
-  /// AtomicExpandPass into an __atomic_* library call.
+  /// as ones that are not correctly aligned, see `atomicAlignmentValid`),
+  /// will be expanded by AtomicExpandPass into an __atomic_* library call.
   unsigned getMaxAtomicSizeInBitsSupported() const {
     return MaxAtomicSizeInBitsSupported;
   }
 
+  /// Check if alignment is valid for atomic operation. The default is
+  /// natural alignment, but there are platforms like x86 where for certain
+  /// sizes different alignments are valid.
+  virtual bool atomicAlignmentValid(unsigned Size, unsigned Align) const {
+    return Align >= Size;
+  }
+
   /// Returns the size of the smallest cmpxchg or ll/sc instruction
   /// the backend supports.  Any smaller operations are widened in
   /// AtomicExpandPass.
   ///
   /// Note that *unlike* operations above the maximum size, atomic ops
   /// are still natively supported below the minimum; they just
   /// require a more complex expansion.
   unsigned getMinCmpXchgSizeInBits() const { return MinCmpXchgSizeInBits; }

lib/CodeGen/AtomicExpandPass.cpp

 
 // Determine if a particular atomic operation has a supported size,
 // and is of appropriate alignment, to be passed through for target
 // lowering. (Versus turning into a __atomic libcall)
 template <typename Inst>
 bool atomicSizeSupported(const TargetLowering *TLI, Inst *I) {
   unsigned Size = getAtomicOpSize(I);
   unsigned Align = getAtomicOpAlign(I);
-  return Align >= Size && Size <= TLI->getMaxAtomicSizeInBitsSupported() / 8;
+  return TLI->atomicAlignmentValid(Size, Align) && Size <= TLI->getMaxAtomicSizeInBitsSupported() / 8;
 }
 
 } // end anonymous namespace
 
 bool AtomicExpand::runOnFunction(Function &F) {
   if (!TM || !TM->getSubtargetImpl(F)->enableAtomicExpand())
     return false;
   TLI = TM->getSubtargetImpl(F)->getTargetLowering();

lib/Target/X86/X86ISelLowering.h

public:
     /// offset as appropriate.
     Value *getSafeStackPointerLocation(IRBuilder<> &IRB) const override;
 
     SDValue BuildFILD(SDValue Op, EVT SrcVT, SDValue Chain, SDValue StackSlot,
                       SelectionDAG &DAG) const;
 
     bool isNoopAddrSpaceCast(unsigned SrcAS, unsigned DestAS) const override;
 
+    bool atomicAlignmentValid(unsigned Size, unsigned Align) const override;
+
     /// \brief Customize the preferred legalization strategy for certain types.
     LegalizeTypeAction getPreferredVectorAction(EVT VT) const override;
 
     bool isIntDivCheap(EVT VT, AttributeSet Attr) const override;
 
     bool supportSwiftError() const override;
 
     unsigned getMaxSupportedInterleaveFactor() const override { return 4; }

lib/Target/X86/X86ISelLowering.cpp

 
 bool X86TargetLowering::isNoopAddrSpaceCast(unsigned SrcAS,
                                             unsigned DestAS) const {
   assert(SrcAS != DestAS && "Expected different address spaces!");
 
   return SrcAS < 256 && DestAS < 256;
 }
 
+bool X86TargetLowering::atomicAlignmentValid(unsigned Size, unsigned Align) const {
+  // Atomic operations of for size < 16 don't need to be aligned on X86
+  if (Size < 16)
+    return true;
+  return Align >= Size;
+}
+
 //===----------------------------------------------------------------------===//
 //               Return Value Calling Convention Implementation
 //===----------------------------------------------------------------------===//
 
 #include "X86GenCallingConv.inc"
 
 bool X86TargetLowering::CanLowerReturn(
     CallingConv::ID CallConv, MachineFunction &MF, bool isVarArg,

test/CodeGen/X86/atomic-load-store-wide.ll

 ; RUN: llc < %s -mcpu=corei7 -march=x86 -verify-machineinstrs | FileCheck %s
 
 ; 64-bit load/store on x86-32
 ; FIXME: The generated code can be substantially improved.
 
 define void @test1(i64* %ptr, i64 %val1) {
 ; CHECK-LABEL: test1
 ; CHECK: lock cmpxchg8b
 ; CHECK-NEXT: jne
   store atomic i64 %val1, i64* %ptr seq_cst, align 8
   ret void
 }
 
-define i64 @test2(i64* %ptr) {
+define void @test2(i64* %ptr, i64 %val1) {
 ; CHECK-LABEL: test2
 ; CHECK: lock cmpxchg8b
+; CHECK-NEXT: jne
+  store atomic i64 %val1, i64* %ptr seq_cst, align 4
+  ret void
+}
+
+define i64 @test3(i64* %ptr) {
+; CHECK-LABEL: test3
+; CHECK: lock cmpxchg8b
   %val = load atomic i64, i64* %ptr seq_cst, align 8
   ret i64 %val
 }
+
+define i64 @test4(i64* %ptr) {
+; CHECK-LABEL: test4
+; CHECK: lock cmpxchg8b
+  %val = load atomic i64, i64* %ptr seq_cst, align 4
+  ret i64 %val
+}