[cfe-commits] [PATCH] Smarter implicit copy-construction/copy-assignment.

[John McCall]

On Jan 11, 2013, at 7:05 PM, Lang Hames <lhames at gmail.com> wrote:
> At present, if a class contains any Non-POD members we perform element-wise copies for each field when generating the implicit copy-constructor and implicit copy-assignment operator (with an optimization for array members).
> 
> This patch changes this behavior - adjacent POD members will be memcpy'd, with Non-POD members output as usual.
> 
> This is an initial implementation that I'd like to get in tree. Obvious deficiencies are:
> 
> It punts entirely when LangOpts.getGC != None.
> It doesn't handle inner classes/unions.
> It doesn't attach any TBAA info, even when all members are the same type.
> It isn't particularly smart about when it falls back on element-wise copies (at the moment it emits a memcpy any time it finds more than one POD field).
> 
> These should all be easy to test and remedy once the code's in tree though.
> 
> Does anybody see any problems with this?
> Style comments?
> 
> Feedback much appreciated.

+void CodeGenFunction::EmitAggregateCopy(llvm::Value *DestPtr,
+                                        llvm::Value *SrcPtr,
+                                        CharUnits Size, CharUnits Alignment){

Spacing on ){.  Also, you might as well call this emitMemcpy, and I'm not sure
why you made it a method on CodeGenFunction, because I actually don't want
people accidentally using it instead of the proper one just because it's more
convenient.

As Richard mentioned, please prefer lowercase function names in new code.

+      QualType FieldType = F->getType();
+      if (FieldType.isVolatileQualified() || FieldType.isObjCGCWeak() ||
+          FieldType.isObjCGCStrong() ||
+          FieldType.hasStrongOrWeakObjCLifetime())
+        return false;

Please grab the Qualifiers once and then query it.

Also, GC qualification is unfortunately more complicated than just
querying the type.  It is not worth trying to get this right; just abandon this
optimization in either of the GC-enabled modes.

+    void AddMemcpyableField(FieldDecl *F) {
+      if (FirstField == 0) {
+        FirstField = F;
+        LastField = F;
+        FirstFieldOffset = RecLayout.getFieldOffset(F->getFieldIndex());
+        LastFieldOffset = FirstFieldOffset;
+        LastAddedFieldIndex = F->getFieldIndex();
+        return;
+      }
+
+      assert(F->getFieldIndex() == LastAddedFieldIndex + 1 &&
+             "Cannot aggregate non-contiguous fields.");
+      LastAddedFieldIndex = F->getFieldIndex();
+
+      // The 'first' and 'last' fields are chosen by offset, rather than field
+      // index. This allows the code to support bitfields, as well as regular
+      // fields.
+      uint64_t FOffset = RecLayout.getFieldOffset(F->getFieldIndex());
+      if (FOffset < FirstFieldOffset) {
+        FirstField = F;
+        FirstFieldOffset = FOffset;
+      }
+      if (FOffset > LastFieldOffset) {
+        LastField = F;
+        LastFieldOffset = FOffset;
+      }
+    }

I feel confident that you can organize this so that it is not quite so
much of a giant blob of code.

    CharUnits GetMemcpySize() const {
+      unsigned LastFieldSize =
+        CGF.getContext().getTypeInfo(LastField->getType()).first; 
+      uint64_t MemcpySizeBits =
+        LastFieldOffset + LastFieldSize - FirstFieldOffset;
+      CharUnits MemcpySize =
+        CGF.getContext().toCharUnitsFromBits(MemcpySizeBits);
+      return MemcpySize;

Do you need to round up here?

+    CodeGenFunction& GetCGF() { return CGF; }
+
+    const CodeGenFunction& GetCGF() const { return CGF; }

Just make CGF a protected member.

+    bool MemberInitIsMemcpyable(CXXCtorInitializer *MemberInit) const {

isMemberInitMemcpyable

+    virtual bool BailOut() = 0;
+

An excellent place to document the meaning of the return type.

+      // Bail out on non-POD, not-trivially-constructable members.
+      if (!FieldType.isPODType(GetCGF().getContext()) &&
+          (!CE || !CE->getConstructor()->isTrivial()))
+        return false;

If you're calling a trivial copy/move constructor, it does not matter
whether the type is POD.  Test case: give a field a type with a trivial
copy constructor and non-trivial destructor, or more realistically a
non-trivial default constructor.

+    void EmitConstructorCopy(CXXCtorInitializer *MemberInit) {

addMemberInitializer

+        // If MemberInit can't be rolled into the memcpy, emit a memcpy for the
+        // currently aggregated fields, then emit an initializer for MemberInit.
+        EmitMemcpy();
+        AggregatedInits.clear();

Shouldn't EmitMemcpy() have an invariant that AggregatedInits
is empty afterwards?

+    virtual bool BailOut() {
+      if (AggregatedStmts.size() == 1) {
+        for (unsigned i = 0; i < AggregatedStmts.size(); ++i)
+          GetCGF().EmitStmt(AggregatedStmts[i]);
+        return true;
+      }
+      return false;
+    }

Restructure to reduce nesting.

+  assert(RootS->getStmtClass() == Stmt::CompoundStmtClass &&
+         "Body of an implicit assignment operator should be compound stmt.");
+  const CompoundStmt &RootCS = cast<CompoundStmt>(*RootS);

The body of this assert is just isa<CompoundStmt>(RootS).
Also, please leave it as a pointer instead of making it a reference.

+//  EmitFunctionBody(Args);

Commented-out code.

+  } else if (FD->isImplicit() && isa<CXXMethodDecl>(FD) &&
+             cast<CXXMethodDecl>(FD)->isCopyAssignmentOperator()) {

You should be checking isImplicitlyDefined(), not isImplicit().  C++11
test case:  A &operator=(const A&) = default;

Either a copy or a move assignment would be fine.  Also, knowing
that it's an implicit assignment operator is sufficient;  you can just
test (FD->getOverloadedOperator() == OO_Equal).

John.