Bugfix for Codegen of atomic load/store ops.

[ABataev]

Summary

Currently clang fires assertions on x86-64 on atomic load/store operations for long double operands. Patch fixes codegen for such operations.

[rjmccall]

Thanks for taking this on. Please test all the basic atomic operations, not just loads and stores, and ensure that the extra bits take on some sensible behavior.

test/CodeGen/x86_64-atomic-long_double.c

7

You need to be more specific in this test. You should be able to match the alloca.

14

This isn't good enough; x86_fp80's store size doesn't fill its rounded-up size. So you also need to ensure that the extra bits have some consistent value — i.e. zero. In other words, there needs to be a memset of the alloca to zero before you can store the long double into it.

[ABataev]

John, thanks for the review! Also I need to add a test for 32 bit mode (long double has size 96 bits there) and check that in this mode we create a libcall.

test/CodeGen/x86_64-atomic-long_double.c

7

Ok, I'll improve test

14

Ok, I'll add additional checks

[ABataev]

Rework after review.
Improved test and fixed bugs in codegen.

[rjmccall]

Hmm. Using the optimized libcalls is a great idea, but it should be a separate patch. You also need to mention that you'd like to do this on cfe-dev first, since it does introduce new runtime dependencies, even if they're a documented part of the existing atomics runtime.

lib/CodeGen/CGAtomic.cpp

1047

Consider making this a method on the AtomicInfo.

1230

Inst->setVolatile(Obj.isVolatileQualified());
Inst->setWeak(IsWeak);

1238

This part should also be a method on the AtomicInfo, or at least its own helper function. convertIntToRValue?

We probably have the same code in the atomic-load path.

[ABataev]

John, Ok, I'll split this patch in 2 parts.

lib/CodeGen/CGAtomic.cpp

1047

Ok, done.

1230

Done

1238

I reworked it.

[ABataev]

Update after review.

[rjmccall]

Thanks for splitting the patch. Generally looks good. One comment:

lib/CodeGen/CGAtomic.cpp

973

Is there a good reason that this has to be conditional rather than just being a single convertIntToRValue method? It's okay to pass a ReturnValueSlot in.

[ABataev]

John, here is my answer

lib/CodeGen/CGAtomic.cpp

973

If some value cannot be bitcasted from Int to ValueType (from Int128 to fp80, for example), we have to create a temp. But we have to create different temps for AtomicLoad() and for AtomicCompareExchange() (here we must create a structure of 2 elements). And these temps must be created only if we were unable to directly cast from int to value type.

[rjmccall]

lib/CodeGen/CGAtomic.cpp

973

Wait, what? Why is EmitAtomicCompareExchange returning a first-class aggregate? Just have it return a std::pair of values. That will also let you return an RValue for the current value, which is probably more generally useful to its callers.

[ABataev]

lib/CodeGen/CGAtomic.cpp

973

Ok, will do

[ABataev]

Update after review

[rjmccall]

Looks great, thanks.

include/clang/Basic/TargetInfo.h

public:
   unsigned getLargeArrayAlign() const { return LargeArrayAlign; }
 
   /// \brief Return the maximum width lock-free atomic operation which will
   /// ever be supported for the given target
   unsigned getMaxAtomicPromoteWidth() const { return MaxAtomicPromoteWidth; }
   /// \brief Return the maximum width lock-free atomic operation which can be
   /// inlined given the supported features of the given target.
   unsigned getMaxAtomicInlineWidth() const { return MaxAtomicInlineWidth; }
+  /// \brief Returns true if the given target supports lock-free atomic
+  /// operations at the specified width and alignment.
+  virtual bool hasBuiltinAtomic(uint64_t AtomicSizeInBits,
+                                uint64_t AlignmentInBits) const {
+    return AtomicSizeInBits <= AlignmentInBits &&
+           AtomicSizeInBits <= getMaxAtomicInlineWidth() &&
+           llvm::isPowerOf2_64(AtomicSizeInBits / getCharWidth());
+  }
 
   /// \brief Return the maximum vector alignment supported for the given target.
   unsigned getMaxVectorAlign() const { return MaxVectorAlign; }
 
   /// \brief Return the size of intmax_t and uintmax_t for this target, in bits.
   unsigned getIntMaxTWidth() const {
     return getTypeWidth(IntMaxType);
   }

lib/CodeGen/CGAtomic.cpp

AtomicInfo(CodeGenFunction &CGF, LValue &lvalue) : CGF(CGF) {
       assert(ValueSizeInBits <= AtomicSizeInBits);
       assert(ValueAlignInBits <= AtomicAlignInBits);
 
       AtomicAlign = C.toCharUnitsFromBits(AtomicAlignInBits);
       ValueAlign = C.toCharUnitsFromBits(ValueAlignInBits);
       if (lvalue.getAlignment().isZero())
         lvalue.setAlignment(AtomicAlign);
 
-      UseLibcall =
-        (AtomicSizeInBits > uint64_t(C.toBits(lvalue.getAlignment())) ||
-         AtomicSizeInBits > C.getTargetInfo().getMaxAtomicInlineWidth());
+      UseLibcall = !C.getTargetInfo().hasBuiltinAtomic(
+          AtomicSizeInBits, C.toBits(lvalue.getAlignment()));
     }
 
     QualType getAtomicType() const { return AtomicTy; }
     QualType getValueType() const { return ValueTy; }
     CharUnits getAtomicAlignment() const { return AtomicAlign; }
     CharUnits getValueAlignment() const { return ValueAlign; }
     uint64_t getAtomicSizeInBits() const { return AtomicSizeInBits; }
-    uint64_t getValueSizeInBits() const { return AtomicSizeInBits; }
+    uint64_t getValueSizeInBits() const { return ValueSizeInBits; }
     TypeEvaluationKind getEvaluationKind() const { return EvaluationKind; }
     bool shouldUseLibcall() const { return UseLibcall; }
 
     /// Is the atomic size larger than the underlying value type?
     ///
     /// Note that the absence of padding does not mean that atomic
     /// objects are completely interchangeable with non-atomic
     /// objects: we might have promoted the alignment of a type
if (atomics.getEvaluationKind() == TEK_Aggregate && resultSlot.isIgnored())
     return RValue::getAggregate(nullptr, false);
 
   // The easiest way to do this this is to go through memory, but we
   // try not to in some easy cases.
   if (atomics.getEvaluationKind() == TEK_Scalar && !atomics.hasPadding()) {
     llvm::Type *resultTy = CGM.getTypes().ConvertTypeForMem(valueType);
     if (isa<llvm::IntegerType>(resultTy)) {
       assert(result->getType() == resultTy);
-      result = EmitFromMemory(result, valueType);
-    } else if (isa<llvm::PointerType>(resultTy)) {
-      result = Builder.CreateIntToPtr(result, resultTy);
-    } else {
-      result = Builder.CreateBitCast(result, resultTy);
-    }
-    return RValue::get(result);
+      return RValue::get(EmitFromMemory(result, valueType));
+    } else if (isa<llvm::PointerType>(resultTy))
+      return RValue::get(Builder.CreateIntToPtr(result, resultTy));
+    else if (llvm::CastInst::isBitCastable(result->getType(), resultTy))
+      return RValue::get(Builder.CreateBitCast(result, resultTy));
   }
 
   // Create a temporary.  This needs to be big enough to hold the
   // atomic integer.
   llvm::Value *temp;
   bool tempIsVolatile = false;
   CharUnits tempAlignment;
   if (atomics.getEvaluationKind() == TEK_Aggregate) {
     assert(!resultSlot.isIgnored());
     temp = resultSlot.getAddr();
llvm::Value *AtomicInfo::materializeRValue(RValue rvalue) const {
 
   // Otherwise, make a temporary and materialize into it.
   llvm::Value *temp = CGF.CreateMemTemp(getAtomicType(), "atomic-store-temp");
   LValue tempLV = CGF.MakeAddrLValue(temp, getAtomicType(), getAtomicAlignment());
   emitCopyIntoMemory(rvalue, tempLV);
   return temp;
 }
 
 /// Emit a store to an l-value of atomic type.
 ///
 /// Note that the r-value is expected to be an r-value *of the atomic
 /// type*; this means that for aggregate r-values, it should include
 /// storage for any padding that was necessary.
 void CodeGenFunction::EmitAtomicStore(RValue rvalue, LValue dest, bool isInit) {
   // If this is an aggregate r-value, it should agree in type except
   // maybe for address-space qualification.
   assert(!rvalue.isAggregate() ||
if (atomics.shouldUseLibcall()) {
     args.add(RValue::get(llvm::ConstantInt::get(
                  IntTy, AtomicExpr::AO_ABI_memory_order_seq_cst)),
              getContext().IntTy);
     emitAtomicLibcall(*this, "__atomic_store", getContext().VoidTy, args);
     return;
   }
 
   // Okay, we're doing this natively.
-  llvm::Value *intValue;
+  llvm::Value *intValue = nullptr;
 
   // If we've got a scalar value of the right size, try to avoid going
   // through memory.
   if (rvalue.isScalar() && !atomics.hasPadding()) {
     llvm::Value *value = rvalue.getScalarVal();
     if (isa<llvm::IntegerType>(value->getType())) {
       intValue = value;
     } else {
       llvm::IntegerType *inputIntTy =
         llvm::IntegerType::get(getLLVMContext(), atomics.getValueSizeInBits());
-      if (isa<llvm::PointerType>(value->getType())) {
+      if (isa<llvm::PointerType>(value->getType()))
         intValue = Builder.CreatePtrToInt(value, inputIntTy);
-      } else {
+      else if (llvm::BitCastInst::isBitCastable(value->getType(), inputIntTy))
         intValue = Builder.CreateBitCast(value, inputIntTy);
-      }
-    }
-
   // Otherwise, we need to go through memory.
-  } else {
+  if (!intValue) {
     // Put the r-value in memory.
     llvm::Value *addr = atomics.materializeRValue(rvalue);
 
     // Cast the temporary to the atomic int type and pull a value out.
     addr = atomics.emitCastToAtomicIntPointer(addr);
     intValue = Builder.CreateAlignedLoad(addr,
                                  atomics.getAtomicAlignment().getQuantity());
   }
AggValueSlot slot = AggValueSlot::forLValue(dest,
                                         Zeroed ? AggValueSlot::IsZeroed :
                                                  AggValueSlot::IsNotZeroed);
 
     EmitAggExpr(init, slot);
     return;
   }
   }
   llvm_unreachable("bad evaluation kind");
 }

test/CodeGen/x86_64-atomic-long_double.c

+// RUN: %clang_cc1 -triple x86_64-linux-gnu -target-cpu core2 %s -S -emit-llvm -o - | FileCheck %s
+
+long double test_load(_Atomic long double *addr) {
+  // CHECK-LABEL: @test_load
+  // CHECK: bitcast x86_fp80* %{{.+}} to i128*
+  // CHECK: load atomic i128*
+  // CHECK: bitcast x86_fp80* %{{.+}} to i128*
+  // CHECK: store i128 %{{.+}}, i128*
+  // CHECK: load x86_fp80*
+  // CHECK: ret x86_fp80
+  return *addr;
+}
+
+void  test_store(_Atomic long double *addr, long double val) {
+  // CHECK-LABEL: @test_store
+  // CHECK: bitcast x86_fp80* %{{.+}} to i128*
+  // CHECK: load i128*
+  // CHECK: bitcast x86_fp80* %{{.+}} to i128*
+  // CHECK: store atomic i128 %{{.+}}, i128* {{.+}}
+  *addr = val;
+}