yurai007 added inline comments.
================ Comment at: llvm/include/llvm/ADT/FoldingSet.h:328 /// Add* - Add various data types to Bit data. - void AddPointer(const void *Ptr); - void AddInteger(signed I); - void AddInteger(unsigned I); - void AddInteger(long I); - void AddInteger(unsigned long I); - void AddInteger(long long I); - void AddInteger(unsigned long long I); + void AddPointer(const void *Ptr) { + // Note: this adds pointers to the hash using sizes and endianness that ---------------- serge-sans-paille wrote: > xbolva00 wrote: > > yurai007 wrote: > > > serge-sans-paille wrote: > > > > Concerning that inlined part, I expect LTO to close the gap instead of > > > > moving everything to headers. Do we have a policy on that topic? > > > I'm not aware of any LLVM Coding Guidlines policy, probably most related > > > is just general rule: > > > https://llvm.org/docs/CodingStandards.html#include-as-little-as-possible > > > I agree LTO is great when enabled. I just tried to move only small part > > > which matters. > > > Could you please elaborate what are you concerning about? > > > > > > The only potential risks which come to my mind right now are: > > > * increased binary size, however I noticed Clang binary grows only below > > > +0.1% which is acceptable I think. > > > * moving to header part of implementation which is often changed, however > > > AddPointer/AddInteger/ComputeHash were touched last time in 2012. > > > * compile time impact on Clang build time. I confess I didn't compare > > > Clang build times before and after change, but if you like I can. > > > * reduced I-cache hit rate. This is something also I didn't check under > > > perf but I can if you like (not sure how important it is given that we > > > get drops of other metrics). > > Is LLVM / Clang in distro releases even built with LTO / LTO + PGO? > In fedora, LLVM is compiled with LTO. But I agree that's not an assumption we > should make, and thus moving some functions to the header looks ok. Yep, recently there is growing interest in using LTO among distros. I heard about at least Fedora, Ubuntu, Red Hat, Debian and Arch interested in enabling it by default. Except Fedora not sure how it looks like with Clang package, though. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118385/new/ https://reviews.llvm.org/D118385 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits