[PATCH] D87805: [PDB] Merge types in parallel when using ghashing

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

In D87805#2304630 <https://reviews.llvm.org/D87805#2304630>, @aganea wrote:

> Thanks again!
>
> LGTM with a few minor things (please see inlines below).

Thanks!

> In D87805#2299073 <https://reviews.llvm.org/D87805#2299073>, @rnk wrote:
>
>> For the MSVC configuration, what debug info flags are used for the compile? /Zi or /Z7, and is /Yu used?
>
> /Z7 because of the distribution and caching, and /Yc /Yu for locally-compiled files. We also link with a bunch of third-party libs (not ours) that are built with /Zi.

I see. But, you are using unity builds, right? Each /Z7 object will contain the world of STL types (std::string), but because of the unity build, the multiplier (number of objects) isn't as large.



================
Comment at: lld/COFF/DebugTypes.cpp:173
+  // Order dependencies first, but preserve the existing order.
+  std::vector<TpiSource *> deps;
+  std::vector<TpiSource *> objs;
----------------
aganea wrote:
> Isn't all this equivalent to `llvm::stable_sort(instances, [](auto *a, auto *b) { return a->isDependency() < b->isDependency(); });`?
Almost (reverse order), but I need to loop over the sources anyway to count how many dependencies there are to build the ArrayRefs. I figured it was better to write one loop to do both things. Maybe that's too much micro-optimization.


================
Comment at: lld/COFF/PDB.cpp:1575
 }
+Number);
+}
----------------
aganea wrote:
> Remove this line and the one after.
I wonder what is causing this. It seems related to the arcanist + clang-format presubmit, so it gets added when I upload a diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87805