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. >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
