Page MenuHomePhabricator

[WPD] Allow load/save bitcoded index when running opt -wholeprogramdevirt
ClosedPublic

Authored by evgeny777 on Jan 21 2020, 3:46 AM.

Diff Detail

Event Timeline

evgeny777 created this revision.Jan 21 2020, 3:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2020, 3:46 AM
evgeny777 planned changes to this revision.Jan 21 2020, 3:47 AM
evgeny777 updated this revision to Diff 239278.Jan 21 2020, 4:34 AM

Removed WPD resolution from source index asm file.

One issue with taking combined indexes in testing is the handling of module paths when there are locals (won't be hit by the included test which doesn't contain any). E.g. in WPD if we try to promote when the target is a local function, we attempt to get its promoted global name:

// If target is a local function and we are exporting it by
// devirtualizing a call in another module, we need to record the
// promoted name.
Res->SingleImplName = ModuleSummaryIndex::getGlobalNameForLocal(
    TheFn.name(), ExportSummary.getModuleHash(S->modulePath()));

Since the modulePath (hardcoded in the test .ll files) won't be found in the index's module path table we'll get an assert in getModulePath. Until this is solved (e.g. possibly via a regex replacement scheme specified by an option that is applied to the module paths in the index, either during assembly or when reading in the bitcode), I'm reluctant to add facilities to consume combined indexes for testing.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
121–122

Option summary needs update.

127–129

Ditto (and in particular the format being dependent on the suffix).

One issue with taking combined indexes in testing is the handling of module paths when there are locals (won't be hit by the included test which doesn't contain any). E.g. in WPD if we try to promote when the target is a local function, we attempt to get its promoted global name:

Honestly, I don't understand how we can step on this. The piece of code you've shown is from DevirtIndex::trySingleImplDevirt. There is no way opt -wholeprogramdevirt can call it, because it uses DevirtModule::trySingleImplDevirt and the latter doesn't have to deal with internal virtual functions, because they're promoted by ThinLTOBitcodeWriter (in order to export declarations to regular LTO module).

One issue with taking combined indexes in testing is the handling of module paths when there are locals (won't be hit by the included test which doesn't contain any). E.g. in WPD if we try to promote when the target is a local function, we attempt to get its promoted global name:

Honestly, I don't understand how we can step on this. The piece of code you've shown is from DevirtIndex::trySingleImplDevirt. There is no way opt -wholeprogramdevirt can call it, because it uses DevirtModule::trySingleImplDevirt and the latter doesn't have to deal with internal virtual functions, because they're promoted by ThinLTOBitcodeWriter (in order to export declarations to regular LTO module).

Looks like there may not be an issue at this moment. I have a vague concern that this could result in unexpected failures/issues. The other place where we check the modulePath in WPD is in AddCalls, which is also invoked by DevirtModule. However, it is used to determine whether the target function needs exporting, and this return value is ignored when this is called from DevirtModule, because as you note any promotion should already have been done. Not sure if that should previously have asserted a false return value, which would have fired incorrectly if the testing code used a bitcode with a local function. Actually, we won't even call AddCalls in that case, because it is guarded by a non-null return value from ExportSummary->getValueInfo(TheFn->getGUID())), which will be null if TheFn is a local in a testing environment, and therefore no edges will be added to the summary in that case, which is ok if we aren't testing for that.

Would it be worthwhile to add some checking in somewhere that there are no locals in the index read by opt?

Would it be worthwhile to add some checking in somewhere that there are no locals in the index read by opt?

Probably we can assert on !isLocalLinkage(FS->linkage()) when analyzing ExportSummary in DevirtModule::run. Another option is to check presence of regular LTO module in combined index when reading summary.

Would it be worthwhile to add some checking in somewhere that there are no locals in the index read by opt?

Probably we can assert on !isLocalLinkage(FS->linkage()) when analyzing ExportSummary in DevirtModule::run.

You can't just always assert that during WPD though, only those locals used by the importing ThinLTO module are promoted when splitting. See promoteInternals() in ThinLTOBitcodeWriter.cpp. 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.

Another option is to check presence of regular LTO module in combined index when reading summary.

This sounds like a good idea too.

evgeny777 updated this revision to Diff 239908.Jan 23 2020, 8:15 AM

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.

tejohnson accepted this revision.Jan 23 2020, 12:41 PM

lgtm with a couple requests for comments.

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.

llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
747

Comment as to why.

This revision is now accepted and ready to land.Jan 23 2020, 12:41 PM
This revision was automatically updated to reflect the committed changes.