[PATCH] D73094: [WPD] Allow load/save bitcoded index when running opt -wholeprogramdevirt

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm with a couple requests for comments.

In D73094#1836360 <https://reviews.llvm.org/D73094#1836360>, @evgeny777 wrote:

> Added check for regular LTO module in combined summary
>
> > So it might be better to do any checking in opt itself, after reading the index, essentially to enforce that we cannot guarantee that any locals are handled as expected in this testing mode
>
> After some experiments this seems useless to me. Given combined summary I can try identifying local virtual functions using this approach:
>
> 1. Call ModuleSummaryIndex::collectDefinedFunctionsForModule("[Regular LTO]", ...)
> 2. Given GUID of virtual function locate all summaries across all modules
> 3. Check that neither of those summaries has local linkage
>
>   But what's the point? Even if I find such summary this most likely means that either ThinLTOBitcodeWriter is broken or index is intentionally modified.


I can't create a scenario where we could run into an issue currently, if this combined index is only used for WPD exporting, with a properly split and promoted module. My concern is more that in the future someone may try to synthesize a combined index in assembly that has an issue, or due to code changes we could end up trying to look for the GUID of a local that didn't originally need promotion in this code. I was originally envisioning some more simpler minded checking, that simply required there to be no locals in the summary whatsoever, until we can properly handle them by fixing up the module paths when testing, e.g. via an option providing a regex replacement scheme on the module paths when assembling.

But since the risk in this particular usage of a combined index seems very low to non-existent at the moment, please go ahead but add a comment somewhere, maybe where we are reading the index, that we assume that any values that WPD cares about will have been properly promoted to global scope.



================
Comment at: llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp:747
+static Error checkCombinedSummaryForTesting(ModuleSummaryIndex *Summary) {
+  // Check that summary index contains regular LTO module
+  const auto &ModPaths = Summary->modulePaths();
----------------
Comment as to why.


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

https://reviews.llvm.org/D73094