[WIP] [CodeGen] Allow specifying Extend to CoerceAndExpand

[rogfer01]

Summary

This is WIP and it is motivated by the suggestions in http://lists.llvm.org/pipermail/cfe-dev/2018-June/058263.html

First attempt, piggybacking the extend information in a structure where the bit-width of the integer represents the exact extension intended. There is a bit of overlapping with the original Extend but I'm not convinced we win anything from rewriting it into the new representation.

I plan to test this using unit tests but I can link a diff with the current usage in case it helps.

[rogfer01]

include/clang/CodeGen/CGFunctionInfo.h

469

I think this is the right trait here. I spent too much time debugging this :)

[rjmccall]

include/clang/CodeGen/CGFunctionInfo.h

91–94

Hmm. I understand the need to use something uniqued here, but I think it would probably be more natural to at least use a llvm::ConstantDataArray (which could be null if there are no interesting bits) instead of encoding the information into the types of a struct type. That would also make it easy to generalize the elements to an arbitrary flags type.

Also, on 64-bit targets this will increase the size of an ABIArgInfo to four pointers from three. That's fine to the extent that we work with independent ABIArgInfos, but I'm getting a little annoyed at the storage overhead of the array of ABIArgInfos in CGFunctionInfo given that, in the overwhelmingly common case, an ABIArgInfo is no more than a kind and maybe a few of these flags. Maybe there's some reasonable way to optimize the storage of an ABIArgInfo in a CGFunctionInfo so that we only need the extra storage in less-common cases? Like extracting out a base class that's the Kind+Flags and making the main array be an array of those + an optional index into a second trailing array of full ABIArgInfos.

I might be overthinking this, though.

[rogfer01]

include/clang/CodeGen/CGFunctionInfo.h

91–94

Oh, of course, using a value rather than a type is more sensible. I will change this.

The idea of having a lighter ABIArgInfo and an ancillary extra array for it seems sensible. I guess it may depend on how early/late we know the precise kind of ABIArgInfo we need to represent. I haven't looked at this yet, I'll investigate.

469

But today I realised that GCC 4.9 does not implement this so it will have to go away :(

[rjmccall]

include/clang/CodeGen/CGFunctionInfo.h

91–94

CGFunctionInfo should be totally immutable and just ultimately constructed with an array of ABIArgInfo, which seems like a reasonable place to inject some representation tricks.

469

You could gate it on the compiler if you want, maybe extracting a HAS_IS_TRIVIALLY_COPYABLE test macro in llvm/Support/type_traits.h out of the implementation of llvm::isPodLike.

[rogfer01]

ChangeLog:

• Use a ConstantDataArray instead of a struct of types.
• Use LLVM_IS_TRIVIALLY_COPYABLE

[rogfer01]

@rjmccall because we do not want to impact the clients of ABIArgInfo I thought of two possible approaches

1. extend CGFunctionInfo with a third trailing array (now it has two), with as many elements as ABIArgInfo (call it ABIArgExtraInfo) then during the creation of CGFunctionInfo link (somehow, see discussion below) each ABIArgInfoto its corresponding ABIArgExtraInfo.
2. add a dynamic container like a SmallVector<ABIArgExtraInfo, 2> in CGFunctionInfo and allocate ABIArgExtraInfo as needed.

In both cases during getFOO, ABIArgInfo would put the extra (less commonly used) data into the corresponding ABIArgExtraInfo (overflowing object).

In both cases too, we need a way to navigate to our enclosing CGFunctionInfo but strictly only during the getFOO that fills the ABIArgInfo. Getting it from the users is not possible, so we would need a pointer to CGFunctionInfo in ABIArgInfo (set up during the creation of CGFunctionInfo). That said, there is no need of its value all the time, only at the beginning of getFOO. We could reuse its storage to point the overflowing object (if needed, and set it to null otherwise).

Do any of the approaches look any close to what you had in mind? Perhaps I'm missing the point.

Thank you!

[rjmccall]

In D48589#1144976, @rogfer01 wrote:

@rjmccall because we do not want to impact the clients of ABIArgInfo I thought of two possible approaches

1. extend CGFunctionInfo with a third trailing array (now it has two), with as many elements as ABIArgInfo (call it ABIArgExtraInfo) then during the creation of CGFunctionInfo link (somehow, see discussion below) each ABIArgInfoto its corresponding ABIArgExtraInfo.
2. add a dynamic container like a SmallVector<ABIArgExtraInfo, 2> in CGFunctionInfo and allocate ABIArgExtraInfo as needed.

In both cases during getFOO, ABIArgInfo would put the extra (less commonly used) data into the corresponding ABIArgExtraInfo (overflowing object).

In both cases too, we need a way to navigate to our enclosing CGFunctionInfo but strictly only during the getFOO that fills the ABIArgInfo. Getting it from the users is not possible, so we would need a pointer to CGFunctionInfo in ABIArgInfo (set up during the creation of CGFunctionInfo). That said, there is no need of its value all the time, only at the beginning of getFOO. We could reuse its storage to point the overflowing object (if needed, and set it to null otherwise).

Do any of the approaches look any close to what you had in mind? Perhaps I'm missing the point.

Well, there's no point in having a trailing array that's always got the same number of elements as there are ABIArgInfos, because then we're not actually saving any memory. What I was thinking was that the representation of ABIArgInfo doesn't really change when it's being passed around as an independent value, but when it's stored in a CGFunctionInfo, we break it up into two components: one that stores all the stuff that frequently appears in ABIArgInfos (which can go in a trailing array indexed by argument index) and one for all the uncommon information (which can go in a condensed trailing array indexed by nothing externally meaningful). When you ask a CGFunctionInfo for the ABIArgInfo for the kth argument, it has to reassemble it, which it does by getting the common-subset ABIArgInfo, asking whether it needs any extra information, and (if so) pulling that out of the second array. Storing the common-subset data together with an index into the trailing array (actually, this could reasonably just be a 32-bit byte offset from the address point of the CGFunctionInfo, which would be more efficient when recreating the ABIArgInfo and would also let us use the zero offset to mean there's no extra information) should be doable in 64 bits, vs. 192 bits in the current representation, so it's a significant savings even before we start adding new fields to ABIArgInfo.

[rogfer01]

Upstream recently amended the ABI spec so it looks to me this is not going to be needed.

include/clang/CodeGen/CGFunctionInfo.h

 #define LLVM_CLANG_CODEGEN_CGFUNCTIONINFO_H
 
 #include "clang/AST/Attr.h"
 #include "clang/AST/CanonicalType.h"
 #include "clang/AST/CharUnits.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/Type.h"
 #include "llvm/IR/DerivedTypes.h"
+#include "llvm/IR/Constants.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/Support/TrailingObjects.h"
+#include "llvm/Support/type_traits.h"
 #include <cassert>
 
 namespace clang {
 namespace CodeGen {
 
 /// ABIArgInfo - Helper class to encapsulate information about how a
 /// specific C type should be passed to or returned from a function.
 class ABIArgInfo {
enum Kind : uint8_t {
     /// arguments stored in memory and forbids any implicit copies.  When
     /// applied to a return type, it means the value is returned indirectly via
     /// an implicit sret parameter stored in the argument struct.
     InAlloca,
     KindFirst = Direct,
     KindLast = InAlloca
   };
 
+  enum class ExtendKind : unsigned { None = 1, SignExt = 2, ZeroExt = 3 };
+
 private:
+
   llvm::Type *TypeData; // canHaveCoerceToType()
   union {
     llvm::Type *PaddingType; // canHavePaddingType()
     llvm::Type *UnpaddedCoerceAndExpandType; // isCoerceAndExpand()
   };
   union {
-    unsigned DirectOffset;     // isDirect() || isExtend()
-    unsigned IndirectAlign;    // isIndirect()
-    unsigned AllocaFieldIndex; // isInAlloca()
+    llvm::ConstantDataArray *ExtendSeq; // isCoerceAndExpand()
+    unsigned DirectOffset;              // isDirect() || isExtend()
+    unsigned IndirectAlign;             // isIndirect()
+    unsigned AllocaFieldIndex;          // isInAlloca()
   };
   Kind TheKind;
   bool PaddingInReg : 1;
   bool InAllocaSRet : 1;    // isInAlloca()
   bool IndirectByVal : 1;   // isIndirect()
   bool IndirectRealign : 1; // isIndirect()
   bool SRetAfterThis : 1;   // isIndirect()
   bool InReg : 1;           // isDirect() || isExtend() || isIndirect()
   bool CanBeFlattened: 1;   // isDirect()
   bool SignExt : 1;         // isExtend()
 
   bool canHavePaddingType() const {
     return isDirect() || isExtend() || isIndirect() || isExpand();
   }
   void setPaddingType(llvm::Type *T) {
     assert(canHavePaddingType());
     PaddingType = T;
   }
 
   void setUnpaddedCoerceToType(llvm::Type *T) {
     assert(isCoerceAndExpand());
     UnpaddedCoerceAndExpandType = T;
   }
 
-  ABIArgInfo(Kind K)
-      : TheKind(K), PaddingInReg(false), InReg(false) {
+  void setExtendSet(llvm::ConstantDataArray *ES) {
+    assert(isCoerceAndExpand());
+    ExtendSeq = ES;
   }
 
+  ABIArgInfo(Kind K) : TheKind(K), PaddingInReg(false), InReg(false) {}
+
 public:
   ABIArgInfo()
-      : TypeData(nullptr), PaddingType(nullptr), DirectOffset(0),
+      : TypeData(nullptr), PaddingType(nullptr), ExtendSeq(nullptr),
         TheKind(Direct), PaddingInReg(false), InReg(false) {}
 
   static ABIArgInfo getDirect(llvm::Type *T = nullptr, unsigned Offset = 0,
                               llvm::Type *Padding = nullptr,
                               bool CanBeFlattened = true) {
     auto AI = ABIArgInfo(Direct);
     AI.setCoerceToType(T);
     AI.setPaddingType(Padding);
static ABIArgInfo getExpandWithPadding(bool PaddingInReg,
     AI.setPaddingInReg(PaddingInReg);
     AI.setPaddingType(Padding);
     return AI;
   }
 
   /// \param unpaddedCoerceToType The coerce-to type with padding elements
   ///   removed, canonicalized to a single element if it would otherwise
   ///   have exactly one element.
-  static ABIArgInfo getCoerceAndExpand(llvm::StructType *coerceToType,
-                                       llvm::Type *unpaddedCoerceToType) {
+  /// \param extendSet The extension to be applied to the expanded type. It is
+  //    represented by a constant data array of integers where each integer
+  //    is a value of ExtendKind. If null no extension is applied. If not null
+  //    it has as many elements as unpaddedCoerceToType. Only integral-types can
+  //    have an extension other than None.
+  static ABIArgInfo
+  getCoerceAndExpand(llvm::StructType *coerceToType,
+                     llvm::Type *unpaddedCoerceToType,
+                     llvm::ConstantDataArray *extendSet = nullptr) {
 #ifndef NDEBUG
     // Sanity checks on unpaddedCoerceToType.
 
     // Assert that we only have a struct type if there are multiple elements.
     auto unpaddedStruct = dyn_cast<llvm::StructType>(unpaddedCoerceToType);
     assert(!unpaddedStruct || unpaddedStruct->getNumElements() != 1);
 
     // Assert that all the non-padding elements have a corresponding element
#ifndef NDEBUG
     }
 
     // Assert that there aren't extra elements in the unpadded type.
     if (unpaddedStruct) {
       assert(unpaddedStruct->getNumElements() == unpaddedIndex);
     } else {
       assert(unpaddedIndex == 1);
     }
+
+    if (extendSet) {
+      // Assert that the size of the extend struct agrees with the unpadded
+      // struct and it only attempts to extend integral types.
+      if (unpaddedStruct) {
+        assert(extendSet->getNumElements() == unpaddedStruct->getNumElements());
+        unsigned Idx = 0;
+        for (auto EltType : unpaddedStruct->elements()) {
+          uint64_t ExtendType = extendSet->getElementAsInteger(Idx);
+          assert(ExtendType == static_cast<uint64_t>(ExtendKind::None) ||
+                 (ExtendType == static_cast<uint64_t>(ExtendKind::ZeroExt) ||
+                  ExtendType == static_cast<uint64_t>(ExtendKind::SignExt)) &&
+                     EltType->isIntegerTy() &&
+                     "Invalid extend applied to non-integral type");
+          Idx++;
+        }
+      } else {
+        assert(extendSet->getNumElements() == 1);
+        uint64_t ExtendType = extendSet->getElementAsInteger(0);
+        assert(ExtendType == static_cast<uint64_t>(ExtendKind::None) ||
+               (ExtendType == static_cast<uint64_t>(ExtendKind::ZeroExt) ||
+                ExtendType == static_cast<uint64_t>(ExtendKind::SignExt)) &&
+                   unpaddedCoerceToType->isIntegerTy() &&
+                   "Invalid extend applied to non-integral type");
+      }
+    }
 #endif
 
     auto AI = ABIArgInfo(CoerceAndExpand);
     AI.setCoerceToType(coerceToType);
     AI.setUnpaddedCoerceToType(unpaddedCoerceToType);
+    AI.setExtendSet(extendSet);
     return AI;
   }
 
   static bool isPaddingForCoerceAndExpand(llvm::Type *eltType) {
     if (eltType->isArrayTy()) {
       assert(eltType->getArrayElementType()->isIntegerTy(8));
       return true;
     } else {
       return false;
     }
   }
 
+  static ExtendKind getExtendKind(uint64_t I) {
+    ExtendKind EK = static_cast<ExtendKind>(I);
+    if (EK == ExtendKind::None || EK == ExtendKind::SignExt ||
+        EK == ExtendKind::ZeroExt) {
+      return EK;
+    }
+    llvm_unreachable("Unexpected integer kind");
+  }
+
   Kind getKind() const { return TheKind; }
   bool isDirect() const { return TheKind == Direct; }
   bool isInAlloca() const { return TheKind == InAlloca; }
   bool isExtend() const { return TheKind == Extend; }
   bool isIgnore() const { return TheKind == Ignore; }
   bool isIndirect() const { return TheKind == Indirect; }
   bool isExpand() const { return TheKind == Expand; }
   bool isCoerceAndExpand() const { return TheKind == CoerceAndExpand; }
llvm::StructType *getCoerceAndExpandType() const {
     return cast<llvm::StructType>(TypeData);
   }
 
   llvm::Type *getUnpaddedCoerceAndExpandType() const {
     assert(isCoerceAndExpand());
     return UnpaddedCoerceAndExpandType;
   }
 
+  llvm::ConstantDataArray *getExtendSeq() const {
+    assert(isCoerceAndExpand());
+    return ExtendSeq;
+  }
+
   ArrayRef<llvm::Type *>getCoerceAndExpandTypeSequence() const {
     assert(isCoerceAndExpand());
     if (auto structTy =
           dyn_cast<llvm::StructType>(UnpaddedCoerceAndExpandType)) {
       return structTy->elements();
     } else {
       return llvm::makeArrayRef(&UnpaddedCoerceAndExpandType, 1);
     }
#endif
   void setCanBeFlattened(bool Flatten) {
     assert(isDirect() && "Invalid kind!");
     CanBeFlattened = Flatten;
   }
 
   void dump() const;
 };
 
+#if defined(LLVM_IS_TRIVIALLY_COPYABLE)
+static_assert(
+    LLVM_IS_TRIVIALLY_COPYABLE(ABIArgInfo),
+    "ABIArgInfo must be trivially copyable as it is embedded as trailing "
+    "data of CGFunctionInfo");
+#endif
+
 /// A class for recording the number of arguments that a function
 /// signature requires.
 class RequiredArgs {
   /// The number of required arguments, or ~0 if the signature does
   /// not permit optional arguments.
   unsigned NumRequired;
 public:
   enum All_t { All };

lib/CodeGen/CGCall.cpp

case ABIArgInfo::Indirect: {
     // inalloca and sret disable readnone and readonly
     FuncAttrs.removeAttribute(llvm::Attribute::ReadOnly)
       .removeAttribute(llvm::Attribute::ReadNone);
     break;
   }
 
   case ABIArgInfo::CoerceAndExpand:
     break;
-
   case ABIArgInfo::Expand:
     llvm_unreachable("Invalid ABI kind for return argument");
   }
 
   if (const auto *RefTy = RetTy->getAs<ReferenceType>()) {
     QualType PTy = RefTy->getPointeeType();
     if (!PTy->isIncompleteType() && PTy->isConstantSizeType())
       RetAttrs.addDereferenceableAttr(getContext().getTypeSizeInChars(PTy)
void CodeGenModule::ConstructAttributeList(
   // Attach attributes to inalloca argument.
   if (IRFunctionArgs.hasInallocaArg()) {
     llvm::AttrBuilder Attrs;
     Attrs.addAttribute(llvm::Attribute::InAlloca);
     ArgAttrs[IRFunctionArgs.getInallocaArgNo()] =
         llvm::AttributeSet::get(getLLVMContext(), Attrs);
   }
 
+  SmallVector<llvm::AttrBuilder, 4> CoerceAndExpandAttrs(IRFunctionArgs.totalIRArgs());
+  bool CoerceAndExpandHasAttributes = false;
   unsigned ArgNo = 0;
   for (CGFunctionInfo::const_arg_iterator I = FI.arg_begin(),
                                           E = FI.arg_end();
        I != E; ++I, ++ArgNo) {
     QualType ParamType = I->type;
     const ABIArgInfo &AI = I->info;
     llvm::AttrBuilder Attrs;
 
case ABIArgInfo::Indirect: {
 
       // byval disables readnone and readonly.
       FuncAttrs.removeAttribute(llvm::Attribute::ReadOnly)
         .removeAttribute(llvm::Attribute::ReadNone);
       break;
     }
     case ABIArgInfo::Ignore:
     case ABIArgInfo::Expand:
+      break;
     case ABIArgInfo::CoerceAndExpand:
+      if (AI.getExtendSeq()) {
+        // Handle extends in expanded items
+        unsigned FirstIRArg, NumIRArgs;
+        std::tie(FirstIRArg, NumIRArgs) = IRFunctionArgs.getIRArgs(ArgNo);
+        llvm::StructType *CoercionType = AI.getCoerceAndExpandType();
+        for (unsigned I = 0, Ext = 0; I < NumIRArgs; I++) {
+          llvm::Type *EltType = CoercionType->getElementType(I);
+          if (ABIArgInfo::isPaddingForCoerceAndExpand(EltType))
+            continue;
+
+          uint64_t ExtendKind = AI.getExtendSeq()->getElementAsInteger(Ext++);
+          switch (ABIArgInfo::getExtendKind(ExtendKind)) {
+          case ABIArgInfo::ExtendKind::None:
+            break;
+          case ABIArgInfo::ExtendKind::SignExt: {
+            CoerceAndExpandHasAttributes = true;
+            CoerceAndExpandAttrs[FirstIRArg + I].addAttribute(
+                llvm::Attribute::SExt);
+            break;
+          }
+          case ABIArgInfo::ExtendKind::ZeroExt: {
+            CoerceAndExpandHasAttributes = true;
+            CoerceAndExpandAttrs[FirstIRArg + I].addAttribute(
+                llvm::Attribute::ZExt);
             break;
+          }
+          }
+        }
+      }
 
+      break;
     case ABIArgInfo::InAlloca:
       // inalloca disables readnone and readonly.
       FuncAttrs.removeAttribute(llvm::Attribute::ReadOnly)
           .removeAttribute(llvm::Attribute::ReadNone);
       continue;
     }
 
     if (const auto *RefTy = ParamType->getAs<ReferenceType>()) {
for (CGFunctionInfo::const_arg_iterator I = FI.arg_begin(),
     case ParameterABI::SwiftContext:
       Attrs.addAttribute(llvm::Attribute::SwiftSelf);
       break;
     }
 
     if (FI.getExtParameterInfo(ArgNo).isNoEscape())
       Attrs.addAttribute(llvm::Attribute::NoCapture);
 
-    if (Attrs.hasAttributes()) {
+    if (Attrs.hasAttributes() || CoerceAndExpandHasAttributes) {
       unsigned FirstIRArg, NumIRArgs;
       std::tie(FirstIRArg, NumIRArgs) = IRFunctionArgs.getIRArgs(ArgNo);
       for (unsigned i = 0; i < NumIRArgs; i++)
+      {
+        llvm::AttrBuilder CoerceAndExpandMergedAttrs(Attrs);
+        CoerceAndExpandMergedAttrs.merge(CoerceAndExpandAttrs[FirstIRArg + i]);
         ArgAttrs[FirstIRArg + i] =
-            llvm::AttributeSet::get(getLLVMContext(), Attrs);
+            llvm::AttributeSet::get(getLLVMContext(), CoerceAndExpandMergedAttrs);
+      }
     }
   }
   assert(ArgNo == FI.arg_size());
 
   AttrList = llvm::AttributeList::get(
       getLLVMContext(), llvm::AttributeSet::get(getLLVMContext(), FuncAttrs),
       llvm::AttributeSet::get(getLLVMContext(), RetAttrs), ArgAttrs);
 }