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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
