[Clang] Fix SwiftCallingConv's aggregate lowering for _Atomic(_Bool).

[varungandhi-apple]

Summary

Fixes rdar://72999296.

[varungandhi-apple]

I don't know how to test this on the LLVM-side (is there a way to do it?). Here's the Swift side test which trips over right now (but is fixed with this patch) by attempting a zext from an i8 to an i1 in Swift's NativeConventionSchema::mapIntoNative.

+++ b/test/IRGen/Inputs/atomic_bool.h
@@ -0,0 +1 @@
+typedef struct { _Atomic(_Bool) value; } MyAtomicBool;
+++ b/test/IRGen/Inputs/module.modulemap
@@ -23 +23,6 @@ module AutolinkModuleMapLink {
 }
+
+module AtomicBoolModule {
+  header "atomic_bool.h"
+  export *
+}
+++ b/test/IRGen/atomic_bool.swift
@@ -0,0 +1,7 @@
+// RUN: not --crash %target-swift-emit-ir %s -I %S/Inputs
+
+import AtomicBoolModule
+
+public func f() -> MyAtomicBool {
+  return MyAtomicBool()
+}

[rjmccall]

Hmm. The way you'd test it on the Clang side would be to just write a test case that passes such an aggregate and tests that it's passed as an i8. But I'm not sure it's at all unreasonable to pass this as an i1 rather than an i8; seems like something that Swift should handle correctly in its call-lowering code.

[varungandhi-apple]

The way you'd test it on the Clang side would be to just write a test case that passes such an aggregate and tests that it's passed as an i8.

Ah, I just noticed an attribute __attribute__((swiftcall)) that is used in some test cases, I can add a test for it.

---

But I'm not sure it's at all unreasonable to pass this as an i1 rather than an i8; seems like something that Swift should handle correctly in its call-lowering code.

Here is the code in the compiler which is creating the i8.

void addStructField(const clang::FieldDecl *clangField,
                    VarDecl *swiftField) {
  unsigned fieldOffset = ClangLayout.getFieldOffset(clangField->getFieldIndex());
  assert(!clangField->isBitField());
  Size offset(fieldOffset / 8);

  // If we have a Swift import of this type, use our lowered information.
  if (swiftField) { /* snip */ }

  // Otherwise, add it as an opaque blob.
  auto fieldSize = ClangContext.getTypeSizeInChars(clangField->getType());
  return addOpaqueField(offset, Size(fieldSize.getQuantity()));
}

From what I understand, the i8 is actually correct. (I looked at LLVM IR generated by small examples and those seem to use i8 for loads, stores and layout.)

That said, there is special handling in NativeConventionSchema::mapIntoNative and NativeConventionSchema::mapFromNative. Here's the code in mapIntoNative.

if (fromNonNative.size() == 1) {
  auto *elt = fromNonNative.claimNext();
  if (nativeTy != elt->getType()) {
    if (isa<llvm::IntegerType>(nativeTy) &&
        isa<llvm::IntegerType>(elt->getType()))
      elt = IGF.Builder.CreateZExt(elt, nativeTy);

This ends up trying to zext from i8 to i1 and trips over.

Are you suggesting that I change the code in mapIntoNative/mapFromNative to flip the zext/trunc the other way around based on sizes? Should I be adding the special case to addStructField? Something else?

[rjmccall]

Clang is trying to avoid using non-byte-multiple integer types for memory accesses in LLVM IR, whereas Swift isn't. If you take your test case and make the field non-atomic, you'll see that: Clang is accessing the field as an i8 and Swift is accessing it as an i1. So there's a deeper mismatch here which in practice works out because there's no actual difference in memory access width between those types.

The most correct thing for Swift to do is probably to follow Clang's lead in access widths when accessing a C type. In general, I'm not sure there's any compelling reason for Swift to be doing its own IR type layout on Clang's struct types instead of asking Clang IRGen for the lowering, field indexes, and so on. So in the abstract, I think the best fixes would be for Swift to use Clang's IR type and to access this field in memory as an i8, whether it's atomic or not; and if we did that, Swift would have to have mapIntoNative / mapFromNative potentially truncate / extend to match the CC schema, even for fields Swift imported as Bool. That would be a more invasive change; probably right now it'd be better to just add that logic to match schemas so that _Atomic(_Bool) works.

[varungandhi-apple]

Closing this in favor of a Swift-side fix: https://github.com/apple/swift/pull/35488.

clang/lib/CodeGen/SwiftCallingConv.cpp

// Member pointer types.
     addOpaqueData(begin, begin + CGM.getContext().getTypeSizeInChars(type));
 
     // Atomic types.
   } else if (const auto *atomicType = type->getAs<AtomicType>()) {
     auto valueType = atomicType->getValueType();
     auto atomicSize = CGM.getContext().getTypeSizeInChars(atomicType);
     auto valueSize = CGM.getContext().getTypeSizeInChars(valueType);
 
+    auto valueTy = atomicType->getValueType();
+    // _Atomic(_Bool) does not correspond to i1.
+    if (valueTy == CGM.getContext().BoolTy) {
+      addTypedData(llvm::Type::getIntNTy(CGM.getLLVMContext(),
+                                         CGM.getContext().toBits(valueSize)),
+                   begin);
+    } else {
+      addTypedData(atomicType->getValueType(), begin);
+    }
     addTypedData(atomicType->getValueType(), begin);
 
     // Add atomic padding.
     auto atomicPadding = atomicSize - valueSize;
     if (atomicPadding > CharUnits::Zero())
       addOpaqueData(begin + valueSize, begin + atomicSize);
 
     // Everything else is scalar and should not convert as an LLVM aggregate.