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

[Richard Smith]

  Generally this looks good, no really substantive comments.


================
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());
+}
+
----------------
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)?

================
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!");
----------------
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.

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

================
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);
----------------
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.

================
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();
----------------
Unused local var?

================
Comment at: lib/CodeGen/CGDecl.cpp:1648
@@ -1643,4 +1647,3 @@
   llvm::Value *DeclPtr;
-  bool HasNonScalarEvalKind = !CodeGenFunction::hasScalarEvaluationKind(Ty);
-  // If this is an aggregate or variable sized value, reuse the input pointer.
-  if (HasNonScalarEvalKind || !Ty->isConstantSizeType()) {
+  bool doStore = false;
+  CharUnits Align = getContext().getDeclAlign(&D);
----------------
Can you fix the case of this as you move it? =)

================
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");
----------------
Maybe split apart the two conditions here (they share no code other than the call to `hasScalarEvaluationKind`)?

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

================
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();
----------------
Wouldn't you skip this anyway, because it's inreg?

================
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 }>
+
----------------
Should also check that it goes at the correct end of the argument struct.


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