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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

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

> > 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?


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

https://reviews.llvm.org/D73094