<div dir="ltr">Hi John, Richard,<div><br></div><div style>Here's the updated patch for smarter copy-assignment/copy-construction for structs with Non-POD members.</div><div style><br></div><div style>Richard - I've added test cases for volatile fields, inner classes, array members, pod-like members, bitfields and volatile fields.</div>
<div style>I've also pulled the naming scheme in line with the coding standards. I made sure I use ' &' rather than '& ' where appropriate.</div><div style><br></div><div style>John -</div><div style>
<br></div><div class="gmail_extra">1) Qualifiers are now being grabbed once. <br>2) I now fall back on existing codegen for when GCMode != NonGC.</div><div class="gmail_extra">3) Made sure I round-up to the nearest byte when copying bitfields.<br>
</div><div class="gmail_extra">4) I've tried to address the style issues you raised.</div><div class="gmail_extra">5)</div><div class="gmail_extra"><br></div><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> + // Bail out on non-POD, not-trivially-constructable members.</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> + if (!FieldType.isPODType(GetCGF()</span><span style="color:rgb(80,0,80)">.getContext()) &&</span><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> + (!CE || !CE->getConstructor()-></span><span style="color:rgb(80,0,80)">isTrivial()))</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> + return false;</span><br style="color:rgb(80,0,80)">> <br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> If you're calling a trivial copy/move constructor, it does not matter</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> whether the type is POD. Test case: give a field a type with a trivial</span><br style="color:rgb(80,0,80)"><span style="color:rgb(80,0,80)">> copy constructor and non-trivial destructor, or more realistically a</span><br style="color:rgb(80,0,80)">
<span style="color:rgb(80,0,80)">> non-trivial default constructor.</span><br style="color:rgb(80,0,80)"><div class="gmail_extra"><br></div><div class="gmail_extra" style>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.</div>
<div class="gmail_extra"> <br></div><div class="gmail_extra" style>I've also added code to push EH destructors for POD-like classes so that they can be safely memcpy'd as well.</div><div class="gmail_extra" style>
<br></div><div class="gmail_extra" style>Please let me know if there's anything else you'd like to see changed/added.</div><div class="gmail_extra" style><br></div><div class="gmail_extra" style>Cheers,</div><div class="gmail_extra" style>
Lang.</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Feb 1, 2013 at 12:18 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div dir="ltr">Hi Richard, John,<div><br></div><div>Thanks very much for the feedback - an updated patch is in the works.</div>
<div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div></div><div class=""><div class="h5"><div class="gmail_extra">
<br><br><div class="gmail_quote">On Thu, Jan 31, 2013 at 7:55 PM, John McCall <span dir="ltr"><<a href="mailto:rjmccall@apple.com" target="_blank">rjmccall@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div><div>On Jan 11, 2013, at 7:05 PM, Lang Hames <<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>> wrote:<br>
> 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).<br>
><br>
> This patch changes this behavior - adjacent POD members will be memcpy'd, with Non-POD members output as usual.<br>
><br>
> This is an initial implementation that I'd like to get in tree. Obvious deficiencies are:<br>
><br>
> It punts entirely when LangOpts.getGC != None.<br>
> It doesn't handle inner classes/unions.<br>
> It doesn't attach any TBAA info, even when all members are the same type.<br>
> 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).<br>
><br>
> These should all be easy to test and remedy once the code's in tree though.<br>
><br>
> Does anybody see any problems with this?<br>
> Style comments?<br>
><br>
> Feedback much appreciated.<br>
<br>
</div></div>+void CodeGenFunction::EmitAggregateCopy(llvm::Value *DestPtr,<br>
+ llvm::Value *SrcPtr,<br>
+ CharUnits Size, CharUnits Alignment){<br>
<br>
Spacing on ){. Also, you might as well call this emitMemcpy, and I'm not sure<br>
why you made it a method on CodeGenFunction, because I actually don't want<br>
people accidentally using it instead of the proper one just because it's more<br>
convenient.<br>
<br>
As Richard mentioned, please prefer lowercase function names in new code.<br>
<br>
+ QualType FieldType = F->getType();<br>
+ if (FieldType.isVolatileQualified() || FieldType.isObjCGCWeak() ||<br>
+ FieldType.isObjCGCStrong() ||<br>
+ FieldType.hasStrongOrWeakObjCLifetime())<br>
+ return false;<br>
<br>
Please grab the Qualifiers once and then query it.<br>
<br>
Also, GC qualification is unfortunately more complicated than just<br>
querying the type. It is not worth trying to get this right; just abandon this<br>
optimization in either of the GC-enabled modes.<br>
<br>
+ void AddMemcpyableField(FieldDecl *F) {<br>
+ if (FirstField == 0) {<br>
+ FirstField = F;<br>
+ LastField = F;<br>
+ FirstFieldOffset = RecLayout.getFieldOffset(F->getFieldIndex());<br>
+ LastFieldOffset = FirstFieldOffset;<br>
+ LastAddedFieldIndex = F->getFieldIndex();<br>
+ return;<br>
+ }<br>
+<br>
+ assert(F->getFieldIndex() == LastAddedFieldIndex + 1 &&<br>
+ "Cannot aggregate non-contiguous fields.");<br>
+ LastAddedFieldIndex = F->getFieldIndex();<br>
+<br>
+ // The 'first' and 'last' fields are chosen by offset, rather than field<br>
+ // index. This allows the code to support bitfields, as well as regular<br>
+ // fields.<br>
+ uint64_t FOffset = RecLayout.getFieldOffset(F->getFieldIndex());<br>
+ if (FOffset < FirstFieldOffset) {<br>
+ FirstField = F;<br>
+ FirstFieldOffset = FOffset;<br>
+ }<br>
+ if (FOffset > LastFieldOffset) {<br>
+ LastField = F;<br>
+ LastFieldOffset = FOffset;<br>
+ }<br>
+ }<br>
<br>
I feel confident that you can organize this so that it is not quite so<br>
much of a giant blob of code.<br>
<br>
CharUnits GetMemcpySize() const {<br>
+ unsigned LastFieldSize =<br>
+ CGF.getContext().getTypeInfo(LastField->getType()).first;<br>
+ uint64_t MemcpySizeBits =<br>
+ LastFieldOffset + LastFieldSize - FirstFieldOffset;<br>
+ CharUnits MemcpySize =<br>
+ CGF.getContext().toCharUnitsFromBits(MemcpySizeBits);<br>
+ return MemcpySize;<br>
<br>
Do you need to round up here?<br>
<br>
+ CodeGenFunction& GetCGF() { return CGF; }<br>
+<br>
+ const CodeGenFunction& GetCGF() const { return CGF; }<br>
<br>
Just make CGF a protected member.<br>
<br>
+ bool MemberInitIsMemcpyable(CXXCtorInitializer *MemberInit) const {<br>
<br>
isMemberInitMemcpyable<br>
<br>
+ virtual bool BailOut() = 0;<br>
+<br>
<br>
An excellent place to document the meaning of the return type.<br>
<br>
+ // Bail out on non-POD, not-trivially-constructable members.<br>
+ if (!FieldType.isPODType(GetCGF().getContext()) &&<br>
+ (!CE || !CE->getConstructor()->isTrivial()))<br>
+ return false;<br>
<br>
If you're calling a trivial copy/move constructor, it does not matter<br>
whether the type is POD. Test case: give a field a type with a trivial<br>
copy constructor and non-trivial destructor, or more realistically a<br>
non-trivial default constructor.<br>
<br>
+ void EmitConstructorCopy(CXXCtorInitializer *MemberInit) {<br>
<br>
addMemberInitializer<br>
<br>
+ // If MemberInit can't be rolled into the memcpy, emit a memcpy for the<br>
+ // currently aggregated fields, then emit an initializer for MemberInit.<br>
+ EmitMemcpy();<br>
+ AggregatedInits.clear();<br>
<br>
Shouldn't EmitMemcpy() have an invariant that AggregatedInits<br>
is empty afterwards?<br>
<br>
+ virtual bool BailOut() {<br>
+ if (AggregatedStmts.size() == 1) {<br>
+ for (unsigned i = 0; i < AggregatedStmts.size(); ++i)<br>
+ GetCGF().EmitStmt(AggregatedStmts[i]);<br>
+ return true;<br>
+ }<br>
+ return false;<br>
+ }<br>
<br>
Restructure to reduce nesting.<br>
<br>
+ assert(RootS->getStmtClass() == Stmt::CompoundStmtClass &&<br>
+ "Body of an implicit assignment operator should be compound stmt.");<br>
+ const CompoundStmt &RootCS = cast<CompoundStmt>(*RootS);<br>
<br>
The body of this assert is just isa<CompoundStmt>(RootS).<br>
Also, please leave it as a pointer instead of making it a reference.<br>
<br>
+// EmitFunctionBody(Args);<br>
<br>
Commented-out code.<br>
<br>
+ } else if (FD->isImplicit() && isa<CXXMethodDecl>(FD) &&<br>
+ cast<CXXMethodDecl>(FD)->isCopyAssignmentOperator()) {<br>
<br>
You should be checking isImplicitlyDefined(), not isImplicit(). C++11<br>
test case: A &operator=(const A&) = default;<br>
<br>
Either a copy or a move assignment would be fine. Also, knowing<br>
that it's an implicit assignment operator is sufficient; you can just<br>
test (FD->getOverloadedOperator() == OO_Equal).<br>
<span><font color="#888888"><br>
John.<br>
</font></span></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>