[PATCH] D49405: Change the cap on the amount of padding for each vtable to 32-byte (previously it was 128-byte)

[Dean Michael Berris via Phabricator via llvm-commits]

dberris added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:774-777
+    // Cap at 32 was found experimentally to have a good data/instruction
     // overhead tradeoff.
-    if (Padding > 128)
-      Padding = alignTo(InitSize, 128) - InitSize;
+    if (Padding > 32)
+      Padding = alignTo(InitSize, 32) - InitSize;
----------------
zhaomo wrote:
> dberris wrote:
> > Is it possible to have a reference to the experiment results either through a link or a document or in a comment?
> How about a link to the email of the experiment results (http://lists.llvm.org/pipermail/llvm-dev/2018-July/124694.html)? Is the llvm-dev archive considered permanent? 
If it could be summarised somewhere in this file, it would be more permanent and less cryptic than the current comment. I can suggest a summary of the methodology, for example, even citing the trade-off being done. Something like:

```
Experiments with some large applications with a non-trivial number of dynamic types
have shown a binary size reduction of N% with virtually no measurable runtime overhead.
This padding used to be larger (128) and based on more recent experiments (see
<link to email or something in the LLVM docs/ directory>) we found that it could be
reduced to 32 and get significant binary size reductions with comparable runtime
performance.
```

Alternatively, if we can make the number a function of the cache line size (say, instead of 32 use the target's cache line size and derive something like 32 from it) which might/could explain why aligning to these numbers on those boundaries would not cause significant slowdowns but still have good binary size effects/properties. If I read the experiment results I can see that the platforms being built for/tested are 64-bit platforms with (I think) 64-byte cache lines.


Repository:
  rL LLVM

https://reviews.llvm.org/D49405