[cfe-commits] [PATCH] Smarter implicit copy-construction/copy-assignment.
Lang Hames
lhames at gmail.com
Tue Feb 5 17:31:05 PST 2013
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 <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/319c30aa/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smarter_implicit_copies_2.patch
Type: application/octet-stream
Size: 18360 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130205/319c30aa/attachment.obj>
More information about the cfe-commits
mailing list