Address review feedback

Author: andykaylor

clang/lib/CIR/CodeGen/CIRGenCXXABI.h

@@ -37,8 +37,8 @@ class CIRGenCXXABI {
 
   void setCXXABIThisValue(CIRGenFunction &cgf, mlir::Value thisPtr);
 
-  /// Emit a single constructor/destructor with the gien type from a C++
-  /// constructor Decl.
+  /// Emit a single constructor/destructor with the gen type from a C++
+  /// constructor/destructor Decl.
   virtual void emitCXXStructor(clang::GlobalDecl gd) = 0;
 
 public:
@@ -59,7 +59,8 @@ class CIRGenCXXABI {
     return md->getParent();
   }
 
-  /// Return whether the given global decl needs a VTT parameter.
+  /// Return whether the given global decl needs a VTT (virtual table table)
+  /// parameter.
   virtual bool needsVTTParameter(clang::GlobalDecl gd) { return false; }
 
   /// Build a parameter variable suitable for 'this'.
@@ -71,17 +72,6 @@ class CIRGenCXXABI {
   /// Emit constructor variants required by this ABI.
   virtual void emitCXXConstructors(const clang::CXXConstructorDecl *d) = 0;
 
-  /// Insert any ABI-specific implicit parameters into the parameter list for a
-  /// function. This generally involves extra data for constructors and
-  /// destructors.
-  ///
-  /// ABIs may also choose to override the return type, which has been
-  /// initialized with the type of 'this' if HasThisReturn(CGF.CurGD) is true or
-  /// the formal return type of the function otherwise.
-  virtual void addImplicitStructorParams(CIRGenFunction &cgf,
-                                         clang::QualType &resTy,
-                                         FunctionArgList &params) = 0;
-
   /// Returns true if the given constructor or destructor is one of the kinds
   /// that the ABI says returns 'this' (only applies when called non-virtually
   /// for destructors).

clang/lib/CIR/CodeGen/CIRGenClass.cpp

@@ -22,11 +22,10 @@ using namespace clang;
 using namespace clang::CIRGen;
 
 /// Checks whether the given constructor is a valid subject for the
-/// complete-to-base constructor delgation optimization, i.e. emitting the
+/// complete-to-base constructor delegation optimization, i.e. emitting the
 /// complete constructor as a simple call to the base constructor.
 bool CIRGenFunction::isConstructorDelegationValid(
     const CXXConstructorDecl *ctor) {
-
   // Currently we disable the optimization for classes with virtual bases
   // because (1) the address of parameter variables need to be consistent across
   // all initializers but (2) the delegate function call necessarily creates a

clang/lib/CIR/CodeGen/CIRGenFunction.cpp

@@ -527,6 +527,13 @@ void CIRGenFunction::emitConstructorBody(FunctionArgList &args) {
   // complete ctor and then delegate to the base ctor.
 
   assert(!cir::MissingFeatures::emitCtorPrologue());
+  if (ctor->isDelegatingConstructor()) {
+    // This will be handled in emitCtorPrologue, but we should emit a diagnostic
+    // rather than silently fail to delegate.
+    cgm.errorNYI(ctor->getSourceRange(),
+                 "emitConstructorBody: delegating ctor");
+    return;
+  }
 
   // TODO(cir): propagate this result via mlir::logical result. Just unreachable
   // now just to have it handled.
@@ -572,7 +579,7 @@ clang::QualType CIRGenFunction::buildFunctionArgList(clang::GlobalDecl gd,
     args.push_back(param);
 
   if (md && (isa<CXXConstructorDecl>(md) || isa<CXXDestructorDecl>(md)))
-    cgm.getCXXABI().addImplicitStructorParams(*this, retTy, args);
+    assert(!cir::MissingFeatures::cxxabiStructorImplicitParam());
 
   return retTy;
 }

clang/lib/CIR/CodeGen/CIRGenItaniumCXXABI.cpp

@@ -42,9 +42,6 @@ class CIRGenItaniumCXXABI : public CIRGenCXXABI {
   void emitInstanceFunctionProlog(SourceLocation loc,
                                   CIRGenFunction &cgf) override;
 
-  void addImplicitStructorParams(CIRGenFunction &cgf, QualType &retTy,
-                                 FunctionArgList &params) override;
-
   void emitCXXConstructors(const clang::CXXConstructorDecl *d) override;
   void emitCXXStructor(clang::GlobalDecl gd) override;
 };
@@ -82,19 +79,6 @@ void CIRGenItaniumCXXABI::emitInstanceFunctionProlog(SourceLocation loc,
   }
 }
 
-void CIRGenItaniumCXXABI::addImplicitStructorParams(CIRGenFunction &cgf,
-                                                    QualType &retTY,
-                                                    FunctionArgList &params) {
-  const auto *md = cast<CXXMethodDecl>(cgf.curGD.getDecl());
-  assert(isa<CXXConstructorDecl>(md) || isa<CXXDestructorDecl>(md));
-
-  // Check if we need a VTT parameter as well.
-  if (needsVTTParameter(cgf.curGD)) {
-    cgf.cgm.errorNYI(cgf.curFuncDecl->getLocation(),
-                     "addImplicitStructorParams: VTT parameter");
-  }
-}
-
 void CIRGenItaniumCXXABI::emitCXXStructor(GlobalDecl gd) {
   auto *md = cast<CXXMethodDecl>(gd.getDecl());
   auto *cd = dyn_cast<CXXConstructorDecl>(md);
@@ -128,8 +112,9 @@ void CIRGenItaniumCXXABI::emitCXXConstructors(const CXXConstructorDecl *d) {
   }
 }
 
-/// Return whether the given global decl needs a VTT parameter, which it does if
-/// it's a base constructor or destructor with virtual bases.
+/// Return whether the given global decl needs a VTT (virtual table table)
+/// parameter, which it does if it's a base constructor or destructor with
+/// virtual bases.
 bool CIRGenItaniumCXXABI::needsVTTParameter(GlobalDecl gd) {
   auto *md = cast<CXXMethodDecl>(gd.getDecl());
 

clang/test/CIR/CodeGen/ctor.cpp

@@ -33,3 +33,37 @@ void baz() {
 // CHECK-NEXT:     %[[S_ADDR:.*]] = cir.alloca !rec_Struk, !cir.ptr<!rec_Struk>, ["s", init] {alignment = 4 : i64}
 // CHECK-NEXT:     cir.call @_ZN5StrukC1Ev(%[[S_ADDR]]) : (!cir.ptr<!rec_Struk>) -> ()
 // CHECK-NEXT:     cir.return
+
+struct VariadicStruk {
+  int a;
+  VariadicStruk(int n, ...) { a = n;}
+};
+
+void bar() {
+  VariadicStruk s(1, 2, 3);
+}
+
+// When a variadic constructor is present, we call the C2 constructor directly.
+
+// CHECK-NOT: cir.func @_ZN13VariadicStrukC2Eiz
+
+// CHECK:      cir.func @_ZN13VariadicStrukC1Eiz(%arg0: !cir.ptr<!rec_VariadicStruk>
+// CHECK-SAME:                                   %arg1: !s32i
+// CHECK-SAME:                                   ...) {
+// CHECK-NEXT:   %[[THIS_ADDR:.*]] = cir.alloca {{.*}} ["this", init]
+// CHECK-NEXT:   %[[N_ADDR:.*]] = cir.alloca {{.*}} ["n", init]
+// CHECK-NEXT:   cir.store %arg0, %[[THIS_ADDR]]
+// CHECK-NEXT:   cir.store %arg1, %[[N_ADDR]]
+// CHECK-NEXT:   %[[THIS:.*]] = cir.load{{.*}} %[[THIS_ADDR]]
+// CHECK-NEXT:   %[[N:.*]] = cir.load{{.*}} %[[N_ADDR]]
+// CHECK-NEXT:   %[[A_ADDR:.*]] = cir.get_member %[[THIS]][0] {name = "a"}
+// CHECK-NEXT:   cir.store{{.*}} %[[N]], %[[A_ADDR]]
+// CHECK-NEXT:   cir.return
+
+// CHECK:  cir.func @_Z3barv
+// CHECK-NEXT:    %[[S_ADDR:.*]] = cir.alloca !rec_VariadicStruk, !cir.ptr<!rec_VariadicStruk>, ["s", init]
+// CHECK-NEXT:    %[[ONE:.*]] = cir.const #cir.int<1> : !s32i
+// CHECK-NEXT:    %[[TWO:.*]] = cir.const #cir.int<2> : !s32i
+// CHECK-NEXT:    %[[THREE:.*]] = cir.const #cir.int<3> : !s32i
+// CHECK-NEXT:    cir.call @_ZN13VariadicStrukC1Eiz(%[[S_ADDR]], %[[ONE]], %[[TWO]], %[[THREE]])
+// CHECK-NEXT:    cir.return