[PATCH] D118385: [NFC] Optimize FoldingSet usage where it matters

[Dawid Jurczak via Phabricator via cfe-commits]

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