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. 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 <[email protected]> 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 <[email protected]> wrote: > >> On Jan 11, 2013, at 7:05 PM, Lang Hames <[email protected]> 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. >> > >
smarter_implicit_copies_2.patch
Description: Binary data
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
