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

[Varun Gandhi via Phabricator via cfe-commits]

varungandhi-apple added a comment.

> 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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94854/new/

https://reviews.llvm.org/D94854