[PATCH] [ms-cxxabi] Use inalloca on win32 when passing non-trivial C++ objects

[Reid Kleckner]

================
Comment at: lib/CodeGen/CGCall.cpp:532-552
@@ -508,1 +531,23 @@
 
+void CGFunctionInfo::maybeSetArgStructName(const Decl *D) const {
+  assert(ArgStruct);
+
+  // Don't change the name if we already have one.
+  if (ArgStruct->hasName())
+    return;
+
+  // See if this is a real function decl.
+  const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D);
+  if (!FD)
+    return;
+
+  SmallString<256> TypeName;
+  llvm::raw_svector_ostream OS(TypeName);
+  OS << "argmem.";
+  if (FD->getIdentifier())
+    FD->printQualifiedName(OS);
+  else
+    OS << "<anon>";
+  ArgStruct->setName(OS.str());
+}
+
----------------
Richard Smith wrote:
> Seems a bit weird to use the name of the first function with a given type as part of the type of all functions with that type. Have you considered computing the struct type from its constituent types (or just leaving unnamed)?
OK, switched to a literal type.  As discussed, this has the side benefit of looking *much* more like a normal function call, where all the parameter types are supplied.

================
Comment at: lib/CodeGen/CGCall.cpp:1155
@@ +1154,3 @@
+  case ABIArgInfo::InAlloca: {
+    // sret disables readnone and readonly
+    FuncAttrs.removeAttribute(llvm::Attribute::ReadOnly)
----------------
David Majnemer wrote:
> `sret` should be `inalloca`?
Yeah, that seems more accurate.  The semantics of inalloca on the return value are sort of like a double indirect return value.  The sret pointer is stored into the inalloca argument pack.

================
Comment at: lib/CodeGen/CGCall.cpp:1344-1345
@@ -1256,1 +1343,4 @@
 
+  enum ValOrPointer { HaveValue = 0, HavePointer = 1 };
+  typedef llvm::PointerIntPair<llvm::Value *, 1> ValueAndBit;
+  SmallVector<ValueAndBit, 16> ArgVals;
----------------
Richard Smith wrote:
> Maybe either add a comment here saying these values need to match `EmitParmDecl` or change its type to also use the enum. Also, `llvm::PointerIntPair<llvm::Value*, 1, ValOrPointer>` maybe?
Sure.  I didn't specialize it to the enum type to avoid an uncommon instantiation, but that doesn't really matter.

================
Comment at: lib/CodeGen/CGCall.cpp:1569-1570
@@ -1465,1 +1568,4 @@
+#ifndef NDEBUG
+  if (FI.usesInAlloca())
+    ++AI;
   assert(AI == Fn->arg_end() && "Argument mismatch!");
----------------
Richard Smith wrote:
> Can you make a copy of `AI` for this check, or remove the `#ifndef`/`#endif`? (This code ought to be optimized out anyway in `NDEBUG` builds.) Having `AI` different after this block for `NDEBUG` vs non-`NDEBUG` mode is a little scary.
Sure, NDEBUG is gone.

================
Comment at: lib/CodeGen/CGCall.cpp:1933-1935
@@ +1932,5 @@
+
+    // FIXME: Either emit a copy constructor call, or figure out how to do
+    // guaranteed tail calls with perfect forwarding in LLVM.
+    CGM.ErrorUnsupported(param, "non-trivial argument copy for thunk");
+    EmitNullInitialization(Slot.getAddr(), type);
----------------
Richard Smith wrote:
> Let's move ahead with this as-is, but... in every case where we emit a delegate call, we (should) have a code path which does the same thing but duplicates the function body instead (in order to support varargs functions), and we should probably use that codepath in any case where we have an inalloca argument.
> 
> I say "(should)" because this is slightly broken for vtable thunks, and entirely missing for lambda static invoker delegates.
This doesn't work in general because we don't always have the definition of the function we're trying to delegate to available in this TU.

A good example is a pointer to a virtual member function that takes something non-trivial by value.

When we emit a vtable, we might need to emit vtable thunks without having the definition, and we can't rely on the other TU to emit the thunk for us and still claim to be ABI compatible with MSVC.

Basically, duplicating the body isn't a complete solution.  The only complete solution is to add something to LLVM that guarantees a tail call with perfect argument forwarding without requiring fastcc.

================
Comment at: lib/CodeGen/CGCall.cpp:2257-2258
@@ +2256,4 @@
+    assert(Addr);
+    // Don't use an EH cleanup recursively from an EH cleanup.
+    bool useEHCleanupForArray = flags.isForNormalCleanup();
+    const CXXRecordDecl *RD = Ty->getAsCXXRecordDecl();
----------------
Richard Smith wrote:
> Unused local var?
gone

================
Comment at: lib/CodeGen/CodeGenFunction.cpp:590-605
@@ -589,8 +589,18 @@
     ReturnValue = 0;
-  } else if (CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::Indirect &&
+  } else if ((CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::Indirect ||
+              CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::InAlloca) &&
              !hasScalarEvaluationKind(CurFnInfo->getReturnType())) {
     // Indirect aggregate return; emit returned value directly into sret slot.
     // This reduces code size, and affects correctness in C++.
-    ReturnValue = CurFn->arg_begin();
+    if (CurFnInfo->getReturnInfo().getKind() == ABIArgInfo::Indirect) {
+      ReturnValue = CurFn->arg_begin();
+    } else {
+      // Load the sret pointer from the argument struct and return into that.
+      unsigned Idx = CurFnInfo->getReturnInfo().getInAllocaFieldIndex();
+      llvm::Function::arg_iterator EI = CurFn->arg_end();
+      --EI;
+      llvm::Value *Addr = Builder.CreateStructGEP(EI, Idx);
+      ReturnValue = Builder.CreateLoad(Addr, "agg.result");
+    }
   } else {
     ReturnValue = CreateIRTemp(RetTy, "retval");
----------------
Richard Smith wrote:
> Maybe split apart the two conditions here (they share no code other than the call to `hasScalarEvaluationKind`)?
Sure.  Previously I've been told by John to avoid recomputing evaluation kinds, because it's a pretty giant switch.

================
Comment at: lib/CodeGen/TargetInfo.cpp:1060
@@ +1059,3 @@
+
+  // Skip the 'this' parameter in ecx.
+  CGFunctionInfo::arg_iterator I = FI.arg_begin(), E = FI.arg_end();
----------------
Richard Smith wrote:
> Wouldn't you skip this anyway, because it's inreg?
It actually isn't marked inreg.  I'm not sure what would happen if I added inreg.

================
Comment at: lib/CodeGen/TargetInfo.cpp:1026
@@ +1025,3 @@
+                                   ABIArgInfo &Info, QualType Type) const {
+  // Insert padding bytes to respect alignment.
+  unsigned Align = 4U;
----------------
Richard Smith wrote:
> Maybe expand the comment to justify the constant '4' here? =)
Sure

================
Comment at: test/CodeGenCXX/microsoft-abi-byval-sret.cpp:17-18
@@ +16,4 @@
+
+// The sret parameter goes in the argument struct.
+// CHECK: %"argmem.A::foo" = type <{ %struct.A*, %struct.A }>
+
----------------
Richard Smith wrote:
> Should also check that it goes at the correct end of the argument struct.
This should cover that, unless I misunderstand.


http://llvm-reviews.chandlerc.com/D2636