On Jan 11, 2013, at 7:05 PM, Lang Hames <lha...@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.
_______________________________________________
cfe-commits mailing list
cfe-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
