[PATCH] D128097: [Clang] Fix compile time regression caused by D126061.

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

To add to the points that @yurai007 pointed out, I’ve done some more investigation of my own.

**Compile-time regression does not seem to reproduce locally**

Unfortunately, I still haven’t been able to get the test suite running locally. However, I’ve looked at some individual lencod input files which I’m able to compile and which still show a slowdown on http://llvm-compile-time-tracker.com/ even after https://reviews.llvm.org/D128097.

What is confusing to me is that I am not able to reproduce the slowdown. For comparison, here are the instruction counts from http://llvm-compile-time-tracker.com/ and the counts I am measuring locally.

At 7acc88be0312c721bc082ed9934e381d297f4707 <https://reviews.llvm.org/rG7acc88be0312c721bc082ed9934e381d297f4707> (the commit directly before https://reviews.llvm.org/D126061):

| Input file         | Tracker | Own measurement |
| lencod/mb_access.c | 139M    | 162,006,571     |
| lencod/md_low.c    | 227M    | 264,179,977     |
|

At 0d300da799b06931eb6b974198d683548a8c8392 <https://reviews.llvm.org/rG0d300da799b06931eb6b974198d683548a8c8392> (the commit that landed https://reviews.llvm.org/D128097):

| Input file         | Tracker       | Own measurement      |
| lencod/mb_access.c | 141M (+0.88%) | 161,322,974 (-0.42%) |
| lencod/md_low.c    | 228M (+0.57%) | 263,455,713 (-0.27%) |
|

Note that not only the direction of change is different, but also the absolute magnitude of instruction counts.

I built Clang with `-DCMAKE_BUILD_TYPE=Release` and ran Clang with `-O0 -g -c -o /dev/null`. Maybe http://llvm-compile-time-tracker.com/ is doing something slightly different here? However, I suspect the more important factor for the direction of the change may be my local system headers, which are likely different to what http://llvm-compile-time-tracker.com/ is using. At any rate, this is frustrating because I’m not able to reproduce the issue. If it comes to it, would it be possible to obtain a preprocessed version of the input files (i.e. run with `-E`)?

**Overall, compile times have //improved// in terms of cycle count (though not instruction count)**

All of the above may be a moot point because I’ve noticed that the picture looks significantly different when looking at instructions versus cycle counts. The numbers I quoted in an earlier comment are for instruction counts (which is also the metric that @nikic used when originally raising this issue on https://reviews.llvm.org/D126061). Here are the same comparisons I showed earlier, with numbers taken from http://llvm-compile-time-tracker.com, but looking at cycle counts in addition to instruction counts (geomean for -O0 compiles in each case, titles link to the compile time tracker):

Compile-time regression caused by D126061 <http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707&to=8c7b64b5ae2a09027c38db969a04fc9ddd0cd6bb&stat=instructions>
Instructions: +0.35%
Cycles: +0.30%

Improvement due to this patch (D128097) <http://llvm-compile-time-tracker.com/compare.php?from=1c2b756cd6f9f9408863fb0e91f55731f81b46d9&to=0d300da799b06931eb6b974198d683548a8c8392&stat=instructions>
Instructions: -0.18%
Cycles: -1.54%

Cumulative change from before D126061 landed until after this patch (D128097) landed <http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707&to=0d300da799b06931eb6b974198d683548a8c8392&stat=instructions>
Instructions: +0.22%
Cycles: -1.09%

So in terms of cycle count, which is arguably the more important metric, the overall effect of the two changes is over 1% improvement in compile times. It would of course be great if we could reduce the regression on instruction counts too, but given that we have a marked improvement in cycle count, I think that may already be an acceptable result?

@nikic, I would appreciate your input here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128097/new/

https://reviews.llvm.org/D128097