akhuang added inline comments.
================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:2292-2300
+ bool hasCtor = false;
+ for (const auto *Ctor : RD->ctors()) {
if (Ctor->isTrivial() && !Ctor->isCopyOrMoveConstructor())
return false;
+ if (!Ctor->isCopyOrMoveConstructor())
+ hasCtor = true;
+ }
----------------
rsmith wrote:
> dblaikie wrote:
> > rsmith wrote:
> > > This looks pretty similar to:
> > >
> > > ```
> > > return RD->hasUserDeclaredConstructor() &&
> > > !RD->hasTrivialDefaultConstructor();
> > > ```
> > >
> > > (other than its treatment of user-declared copy or move constructors),
> > > but I have to admit I don't really understand what the "at least one
> > > constructor and no trivial or constexpr constructors" rule aims to
> > > achieve, so it's hard to know if this is the right interpretation. The
> > > rule as written in the comment above is presumably not exactly right --
> > > all classes have at least one constructor, and we're not excluding
> > > classes with trivial copy or move constructors, only those with trivial
> > > default constructors.
> > >
> > > I wonder if the intent would be better captured by "at least one
> > > non-inline constructor" (that is, assuming all declared functions are
> > > defined, there is at least one translation unit in which a constructor is
> > > defined that can be used as a home for the class info).
> > So the general goal is to detect any type where the construction of that
> > type somewhere must invoke a constructor that will be IR-generated.
> >
> > Move and copy constructors are ignored because the assumption is they must
> > be moving/copying from some other object, which must've been constructed,
> > ultimately, by a non-move/copy constructor.
> >
> > Ideally this would be usable even for inline ctors - even if the ctor calls
> > get optimized away later[^1], they'd still allow us to reduce the number of
> > places the type is emitted to only those places that call the ctor.
> >
> >
> >
> > [^1] actually, the way that should work doesn't seem to be working right
> > now (eg:
> > type.cpp
> > ```
> > struct t1 { t1() { } };
> > void f1(void*);
> > int main() {
> > f1(new t1());
> > }
> > ```
> > type2.cpp
> > ```
> > struct t1 { t1() { } };
> > void f1(void* v) {
> > delete (t1*)v;
> > }
> > ```
> > build: `clang++ type.cpp -g -Xclang -fuse-ctor-homing type2.cpp &&
> > llvm-dwarfdump a.out`
> > -> definition of "t1" in the DWARF
> > build with optimizations: `clang++ -O3 type.cpp -g -Xclang
> > -fuse-ctor-homing type2.cpp && llvm-dwarfdump a.out`
> > -> missing definition of "t1"
> > `type.cpp` is chosen as a home for `t1` because it calls a user-defined
> > ctor, but then that ctor gets optimized away and there's no other mention
> > of `t1` in `type.cpp` so the type is dropped entirely. This could happen
> > even with a non-inline definition - under LTO the ctor could get optimized
> > away (though this would be safe in FullLTO - the other references to `t1`
> > would be made to refer to the definition and keep it alive - but in ThinLTO
> > the TU defining the ctor might be imported and nothing else - leaving the
> > type unused/dropped)
> > To fix this we should put 'homed' types in the retained types list so they
> > are preserved even if all other code/references to the type are dropped. I
> > think I implemented this homed type pinning for explicit template
> > specialization definitions, because they have no code attachment point, so
> > similar logic could be used for ctor homing. (vtable homing /might/ benefit
> > from this with aggressive/whole program devirtualization? Not sure - harder
> > to actually optimize away all the references to a type, but possible maybe?)
> Oh, I see, that's clever and very neat.
>
> So the intent is to identify types for which we know that either no instances
> are ever created, or some constructor must be actually emitted in some
> translation unit (prior to optimizations running). And that's why we want to
> ignore copy and move constructors [and could in fact ignore any constructor
> taking the class type by value or reference, directly or indirectly] (they
> cannot be the only place that creates an object) and also why we don't want
> to do this if there's a constexpr constructor or trivial default constructor
> (we might not generate code for them). And the "no constructors" check seems
> to effectively be checking whether aggregate initialization could be
> performed.
>
> I think we can replace the loop with `return !RD->isAggregate() &&
> !RD->hasTrivialDefaultConstructor();`.
>
> (Minor aside, it is possible to create an instance of a type if its only
> constructor is a copy constructor, eg with `struct A { A(const A&) {} } a =
> a;`, but I doubt that's a practical concern for your purposes.)
> So the intent is to identify types for which we know that either no instances
> are ever created, or some constructor must be actually emitted in some
> translation unit (prior to optimizations running). And that's why we want to
> ignore copy and move constructors [and could in fact ignore any constructor
> taking the class type by value or reference, directly or indirectly] (they
> cannot be the only place that creates an object) and also why we don't want
> to do this if there's a constexpr constructor or trivial default constructor
> (we might not generate code for them). And the "no constructors" check seems
> to effectively be checking whether aggregate initialization could be
> performed.
> I think we can replace the loop with return !RD->isAggregate() &&
> !RD->hasTrivialDefaultConstructor();.
Ok, I think that makes sense - I guess the loop was sort of attempting to do
what `RD->hasTrivialDefaultConstructor()`does.
> To fix this we should put 'homed' types in the retained types list so they
> are preserved even if all other code/references to the type are dropped.
Yeah, think you mentioned this before and then I never added it--I'll make a
separate patch for it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D87808/new/
https://reviews.llvm.org/D87808
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
