ahatanak marked 2 inline comments as done.
ahatanak added inline comments.
================
Comment at: lib/CodeGen/CGExprAgg.cpp:315
+ }
+ }
}
----------------
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.
================
Comment at: lib/CodeGen/CodeGenCStruct.cpp:27
+ FK_Array // array that has non-trivial elements.
+};
+}
----------------
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > There's no need to put this in an anonymous namespace.
> > >
> > > This enum makes me wonder if it would make more sense to create
> > > equivalents to QualType::DestructionKind for each of these operations and
> > > all of the different primitive types. That would let you e.g. implement
> > > your only-strong-members check above much more easily, and it would also
> > > make it simpler to guarantee that all the places that need to
> > > exhaustively enumerate the reasons why something might need special
> > > initialization/copying logic don't miss an important new case.
> > I'm not sure if I understood your point here. Do you mean there should be
> > DestructionKinds for arrays or structs too or merge these enums into
> > DestructionKind?
> I was suggesting there should be equivalents to DestructionKind for some of
> the other operations, like a NonTrivialCopyKind, and queries on QualType
> analogous to isDestructedType(), e.g. isNonTrivialToCopy(). That way you can
> directly put this stuff in the AST, and then here you can just walk the
> struct fields and switch on the enum, at which point I think you don't need
> FieldKind at all. Remember that some types are non-trivial in only a subset
> of these dimensions.
I added enum NonTrivialCopyKind and function isNonTrivialToCopy to QualType and
removed FieldKind from CGNonTrivialStruct.cpp. Since there are special
functions other than copy constructor/assignment operator, should the enum's
name be NonTrivialStructKind or something instead of NonTrivialCopyKind?
https://reviews.llvm.org/D41228
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
