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

[Richard Smith]

On Tue, Feb 5, 2013 at 5:31 PM, Lang Hames <lhames at gmail.com> wrote:

> Hi John, Richard,
>
> Here's the updated patch for smarter copy-assignment/copy-construction for
> structs with Non-POD members.
>
> Richard - I've added test cases for volatile fields, inner classes, array
> members, pod-like members, bitfields and volatile fields.
> I've also pulled the naming scheme in line with the coding standards. I
> made sure I use ' &' rather than '& ' where appropriate.
>
> John -
>
> 1) Qualifiers are now being grabbed once.
> 2) I now fall back on existing codegen for when GCMode != NonGC.
> 3) Made sure I round-up to the nearest byte when copying bitfields.
> 4) I've tried to address the style issues you raised.
> 5)
>
>
> > +      // 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.
>
> Note that this is the test for bailing out. The POD check needs to be
> there for primitive types, for which CE == 0, otherwise we'd decide that we
> couldn't memcpy primitive fields.
>

Just because the type is POD, doesn't mean the selected constructor is
trivial. Counterexample:

struct S { S() = default; template<typename T> S(T&&); S(const S&) =
default; };
struct U { U(); S s; };
static_assert(__is_pod(S), "");
U u = static_cast<U&&>(U()); // static_cast to disable copy elision

S is POD, but U's move constructor calls S's constructor template.

I think this is what you mean:

  CE ? CE->getConstructor()->isTrivial() :
FieldType.isTriviallyCopyableType(Ctx)

 I've also added code to push EH destructors for POD-like classes so that
> they can be safely memcpy'd as well.
>
> Please let me know if there's anything else you'd like to see
> changed/added.
>
> Cheers,
> Lang.
>
> On Fri, Feb 1, 2013 at 12:18 PM, Lang Hames <lhames at gmail.com> wrote:
>
>> 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/20130205/66518a2f/attachment.html>