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

[Jonathan Sauer]

Hello,

>> 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.
Or am I overlooking something?

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:

| 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 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:

struct Big {
  NonPodWithTailPadding a;
  NonPodWithTailPadding b;
} big;

As of now copying <big> results in two llvm.memcpy calls to copy <a> and <b> including
(erroneously) their tail padding. As these memcpys copy two neighboring memory blocks, the
optimizer might be able to merge them into a single bigger memcpy (although I don't know
if it actually performs this optimization).

After the patch, <a> and <b> would be copied without tail padding (even though it wouldn't
hurt in this case), so the optimizer would see two memcpys of unconnected memory blocks
and couldn't merge them.


What do you think?

Jonathan