On Jul 17, 2012, at 4:02 AM, Jonathan Sauer wrote: >>> This is definitely the right approach. A few specific comments: >>> >>> - This change should affect the the EmitGCMemmoveCollectable call as well, >>> since the same bug exists there for Objective-C++ GC >>> - There are other CreateMemCpy calls in IR generation that likely need this >>> same computation. Please factor this computation into a separate routine and >>> update those callers >>> - Please include a test case that generates IR and checks that it does the >>> right thing >>> >>> Thanks for working on this! >> >> I actually fixed this in 153613, as a fix for PR12204. I was then asked >> to revert my change because it caused significant compile-time >> regressions under a bootstrap build. My previous commit is still >> correct, but I haven't had time to revisit it and perform the necessary >> performance testing; if Jonathan would like to do so, that would be >> great. > > I have been looking into your patch. As far as I can see, you committed it as > r153613, which was then reverted by Chad Rosier in r153660. Afterwards, I have > not found any new commit by you regarding this change, which leads me to > assume > that PR122024 isn't actually fixed as of now, making PR13329 a duplicate of > it.
That's correct. > As far the patch itself, I don't think it is taking the right approach. > According > to <http://sourcery.mentor.com/public/cxx-abi/cxx-closed.html#A9>, the > deciding > factor on if a base class's tail padding can be used by a derived class to > store > its members in is whether the base class is a POD or not. This is detailed in > <http://sourcery.mentor.com/public/cxx-abi/abi.html> with (among other things) > the following quote: You're right that my patch doesn't actually check POD-ness before doing all this. In principle, that could matter, and I agree that we should do the check. In practice, it's somewhat unlikely that a non-POD class would have a trivial copy assignment operator. > | Because the C++ standard requires that compilers not overlay the tail > padding > | in a POD > > Unfortunately the ABI spec doesn't mention *where* in the C++ standard this > requirement is codified; I was only able to find a footnote in section 5.3.3/ > expr.sizeof: > > | The actual size of a base class subobject may be less than the result of > | applying sizeof to the subobject, due to virtual base classes and less > strict > | padding requirements on base class subobjects. > > But anyway: The Itanium ABI requires reusing tail padding, clang does this no > matter which ABI is used and also handles PODs correctly by *not* reusing > their > tail padding. And it's definitely a useful space optimization. > > TL;DR: IMO POD-ness should decide on how to copy an object with llvm.memcpy, > which > is what I did in my patch after my initial blunder where I simply disabled > llvm.memcpy. I agree, and most of the complexity in my patch is devoted to tracking whether we can get away with a full sizeof memcpy, pretty much for the reasons you give below. > I don't think it would result in significant compile-time regressions, > although > run-time performance might suffer a bit as the optimizer couldn't merge the > memcpys of > several neighboring class members anymore: I specifically said a bootstrap compile-time regression. I was not able to duplicate the regression without bootstrap, so I assume we were seeing a runtime performance hit in the phase 2 compiler. I didn't have time to track down the bootstrap performance problem. It's possible that it would go away if my patch was checking for non-POD-ness. John. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
