rsmith added a comment. In D87808#2288186 <https://reviews.llvm.org/D87808#2288186>, @dblaikie wrote:
> In D87808#2286664 <https://reviews.llvm.org/D87808#2286664>, @rsmith wrote: > >> But I think this code should give the same outcome either way, because a >> class with any constructor other than a default/copy/move constructor must >> have a user-declared constructor of some kind, and so will never have an >> implicit default constructor. > > Hmm, trying to parse this. So if we're in the subtle case of having an > implicit default constructor (and no other (necessarily user-declared, as you > say) constructors) - if it's not been declared, then this function will > return false. If it has been declared, it'll return false... hmm, nope, then > it'll return true. Hmm, yes, you're right, if the implicit default constructor would not be trivial, we could get different answers depending on whether we triggered its implicit declaration or not. And for: struct A { A(); }; struct X { A a; }; struct Y { A a; }; void f(X x) {} void f(decltype(Y()) y) {} ... I see we get different debug information for `X` and `Y`, and it doesn't look like this patch will fix that, but querying `hasTrivialDefaultConstructor()` would. ================ 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; + } ---------------- 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.) 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
