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

[Lang Hames]

Hi Richard, John,

Thanks very much for the feedback - an updated patch is in the works.

Cheers,
Lang.



On Thu, Jan 31, 2013 at 7:55 PM, John McCall <rjmccall at apple.com> wrote:

> 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.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130201/ba5a783a/attachment.html>