[PATCH] D41228: [ObjC] Enable __strong pointers in structs under ARC

[Akira Hatanaka via Phabricator via cfe-commits]

ahatanak marked 2 inline comments as done.
ahatanak added inline comments.


================
Comment at: lib/CodeGen/CGExprAgg.cpp:315
+    }
+  }
 }
----------------
ahatanak wrote:
> rjmccall wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > Do these functions have a memcpy as a precondition?  I would expect them to do the full copy (for code size if nothing else).
> > > Yes, there should always be a call to memcpy before the copy/move special functions are called.  I don't think we want to fold the call to memcpy into the body of the special functions since another special function can be called from the body if the non-trivial C struct has a field whose type is a non-trivial C struct too (in which case, there will be a redundant memcpy to copy the C struct field).
> > > 
> > > For example, in the following code, there shouldn't be a call to memcpy to copy field "f0" of StrongOuter if there is already a memcpy that copies struct StrongOuter:
> > > 
> > > ```
> > > typedef struct {
> > >   int f0;
> > >   id f1;
> > > } Strong;
> > > 
> > > typedef struct {
> > >   Strong f0;
> > >   id f1;
> > > } StrongOuter;
> > > ```
> > > 
> > Well, I guess I was imagining something more C++-ish where you don't necessarily have a struct-wide memcpy, and instead you just memcpy the parts where that's profitable and otherwise do something type-specific, which would mean recursing for a struct.  Your approach is reasonable if the non-trivial copying is relatively sparse and the structure is large; on the other hand, if the non-trivial copying is dense, the memcpy itself might be mostly redundant.  And it does mean a bigger code-size hit in the original place that kicks off the copy.
> > 
> > IIRC we do already have some code in copy-constructor emission that's capable of emitting sequences of field copies with a memcpy, if you wanted to try the C++ style, you could probably take advantage of that pretty easily.  But I won't hold up the patch if you think the memcpy precondition is the right way to go.
> I made changes so that either a load/store pair or a call to memcpy is emitted in the copy/move special functions to copy fields that are not non-trivial.
I haven't implemented the optimization you suggested which merges a sequence of field copies into a single memcpy. I'll look into it later.


https://reviews.llvm.org/D41228