[cfe-commits] [Patch][Review] Bug 13329 - Incorrect code generated for automatic assignment operator

[John McCall]

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.