steakhal accepted this revision. steakhal added a comment. This revision is now accepted and ready to land.
In D124462#3482728 <https://reviews.llvm.org/D124462#3482728>, @mantognini wrote: > In D124462#3480348 <https://reviews.llvm.org/D124462#3480348>, @aaron.ballman > wrote: > >> In D124462#3480219 <https://reviews.llvm.org/D124462#3480219>, @steakhal >> wrote: >> >>> Thanks for the stats. >>> @aaron.ballman WDYT, where should we put the `LLVM_DUMP_METHOD` ? >> >> The documentation for the attribute says function definitions, but I don't >> think it matters in terms of the semantics of the attributes. I'd probably >> put it on the definition in this case because the goal is to keep the >> function around at runtime but it doesn't impact the interface of the call >> or how users would use it at compile time. >> >> I noticed that we seem to be pretty bad about this advice however: >> >> /// Note that you should also surround dump() functions with >> /// `#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)` so they do always >> /// get stripped in release builds. > > I don't have a strong opinion on where `LLVM_DUMP_METHOD` should be and > whether this means writing it twice as I can see arguments for both > approaches. > > In addition to not ifdef-out those functions, I note that many `dump` or > `dumpToStream` functions in `Analysis` (and LLVM in general) don't have the > `LLVM_DUMP_METHOD` macro at all. This is because in some cases it makes sense to preserve such dumps, e.g. the CFG dump might be useful not only for debugging but for others as well. Or the liveness analysis stuff and a lot more. For this case specifically, one should not dump these taint metadata in release builds IMO. I've done some measurements to inspect the size increase for preserving some dump methods, and at the time I could not see much difference at all. Check D124442 <https://reviews.llvm.org/D124442>, D124443 <https://reviews.llvm.org/D124443>. They have not landed yet, but I'm planning to land them in the close future. > So I'm tempted to say the overall situation should be improved in a separate > effort, and this patch can focus only on the orthogonal linking issue. WDYT? Definitely. We should consolidate the situation but at another time. LGTM, given that you add the `LLVM_DUMP_METHOD` to any of the declarations. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D124462/new/ https://reviews.llvm.org/D124462 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
