[PATCH] D33790: [ThinLTO] Assign ValueId only once per GUID when writing combined summary

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added a comment.

In https://reviews.llvm.org/D33790#770953, @tejohnson wrote:

> In https://reviews.llvm.org/D33790#770815, @pcc wrote:
>
> > Have you benchmarked this change? It seems that it wouldn't make a difference for the WriteIndexesThinBackend because it always passes a ModuleToSummariesForIndex map.
>
>
> No and you are right it won't really help in writing distributed indexes (I can fix that in the description). I had been hanging on to this change for a couple days and forgot why I wanted it. It just cleans up something strange I noticed when examining this code - that we will repeatedly assign value numbers to summaries with the same GUID (and you end up with non-contiguous value ids, which isn't a problem, just perhaps looks strange). The change will have more impact when writing out the whole combined index, although we don't do that in any production mode with ThinLTO (distributed or otherwise).


Improving performance of a non-production configuration doesn't seem compelling, to be honest. If you just wanted to address non-contiguous value ids, it seems that this would be simpler and would also address that issue in distributed indexes.

  forEachSummary([&](GVInfo I) {
    auto &V = GUIDToValueIdMap[I.first];
    if (!V)
      V = ++GlobalValueId;
  });


https://reviews.llvm.org/D33790