Details
- Reviewers
- None
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Apply an incremental change to avoid pull full repo (slow build!)
Will git rebase later.
| llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp | ||
|---|---|---|
| 961 ↗ | (On Diff #392291) | You want to look it up with the linkage as it is currently, since that was what would have been used to compute the guid when this symbol's summary was written into the index. So just ExportSummary->getValueInfo(TheFn->getGUID()) should suffice. This will also allow you to remove the IsExported parameter here and it's computation in the caller. |
| 973 ↗ | (On Diff #392291) | You'll also want to check Summary->isLive() and ignore any that aren't. If the prevailing symbol is not in the index, for example, the copies in the index should be marked dead and we should ignore the summaries and return false conservatively. |
| llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp | ||
|---|---|---|
| 961 ↗ | (On Diff #392291) | thanks for pointing this out! I added some verbose logging in ModuleSummaryAnalysis.cpp and WholeProgramDevirt.cpp, to log functions GUID before being added around this line (https://github.com/llvm/llvm-project/blob/main/llvm/lib/Analysis/ModuleSummaryAnalysis.cpp#L503), like [1] And also dump the ExportSummary (call dump method) as well as the function GUID in mustBeUnreachableFunction like [2].
Before combining the IsExported information, ExportSummary->getValueInfo(TheFn->getGUID()) would return null, so read https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp#L1158-L1182 and found the linkage is rewritten. Having said that, I might well miss a context here, and using a pre-computed IsExported indeed looks a little weird to me. [1] ` << F.getGlobalIdentifier() << " to global value summary\n"; Index.addGlobalValueSummary(F, std::move(FuncSummary)); [2] // ExportSummary is absent. No sufficient information to determine // if the function is reachable.
if (ExportSummary == nullptr) {
return false;
}
assert((TheFn != nullptr) && "Caller guarantees that TheFn is not nullptr");
errs() << "\tGlobal identifier with current linkage is "
<< TheFn->getGlobalIdentifier() << "\n";
// If a function will be exported, use external linkage to get its
// global identifier.
const std::string rewrittenFuncGlobalIdentifier =
GlobalValue::getGlobalIdentifier(TheFn->getName(),
IsExported ? GlobalValue::ExternalLinkage
: TheFn->getLinkage(),
TheFn->getParent()->getSourceFileName());
errs() << "\tGlobal identifier with rewritten linkage is "
<< rewrittenFuncGlobalIdentifier << "\n";[3] Example output from ModuleSummaryAnalysis
Going to add F _ZN7Derived1xEv with global identifier as _ZN7Derived1xEv to global value summary
[4]Global identifier with current linkage is ld-temp.o:_ZN4BaseD0Ev |
| 973 ↗ | (On Diff #392291) | thanks for pointing this out! I'll dig around a little bit about how isLive is used currently and follow this up soon. |
| llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp | ||
|---|---|---|
| 961 ↗ | (On Diff #392291) |
It seems I cannot edit comment after a while, so just cleaned up the format (by adding a linebreak after[1]) of [1][2][3][4] and posted it . The preview looks ok this time. errs() << "Going to add F " << F.getName() << " with global identifier as "
<< F.getGlobalIdentifier() << " to global value summary\n";
Index.addGlobalValueSummary(F, std::move(FuncSummary));[2] // ExportSummary is absent. No sufficient information to determine
// if the function is reachable.
if (ExportSummary == nullptr) {
return false;
}
assert((TheFn != nullptr) && "Caller guarantees that TheFn is not nullptr");
errs() << "\tGlobal identifier with current linkage is "
<< TheFn->getGlobalIdentifier() << "\n";
// If a function will be exported, use external linkage to get its
// global identifier.
const std::string rewrittenFuncGlobalIdentifier =
GlobalValue::getGlobalIdentifier(TheFn->getName(),
IsExported ? GlobalValue::ExternalLinkage
: TheFn->getLinkage(),
TheFn->getParent()->getSourceFileName());
errs() << "\tGlobal identifier with rewritten linkage is "
<< rewrittenFuncGlobalIdentifier << "\n";[3] Example output from ModuleSummaryAnalysis Going to add F _ZN7Derived1xEv with global identifier as _ZN7Derived1xEv to global value summary [4] Global identifier with current linkage is ld-temp.o:_ZN4BaseD0Ev Global identifier with rewritten linkage is _ZN4BaseD0Ev Global identifier with current linkage is ld-temp.o:_ZN7DerivedD0Ev Global identifier with rewritten linkage is _ZN7DerivedD0Ev |
update based on the hint from
"https://buildkite.com/llvm-project/diff-checks/builds/78250"
| llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp | ||
|---|---|---|
| 961 ↗ | (On Diff #392291) | I suspect this is due to regular LTO internalization which has already updated the linkage types in the IR much earlier, and that we are sort of getting lucky that we are going to re-export due to WPD which undoes the effect of the earlier internalization. To confirm, you could try disabling LTO internalization and see if that causes the global identifier printed in the "current linkage" message above to not have the path in it. To do so, use -Wl,-mllvm,-enable-lto-internalization=false. If it is internalization like I suspect, I think the easier and more robust thing (rather than relying on checking whether WPD will export, which may not be correlated to the earlier internalization decision), would be to simply lookup in the index with the exported name unconditionally if there is no entry found with the current linkage. I.e. something like: ValueInfo TheFnVI = ExportSummary->getValueInfo(TheFn->getGUID()); if (!TheFnVI) // Assume TheFn was internalized, lookup index with original name. TheFnVI = ExportSummary->getValueInfo(GlobalValue::getGUID(TheFn->getName()); |
| llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp | ||
|---|---|---|
| 961 ↗ | (On Diff #392291) | And we can guard the second index lookup on whether TheFn->getGUID() != GlobalValue::getGUID(TheFn->getName()). Probably save the GUIDs in locals so we don't have to repeat the hash computation. And I think we should be able to assert that TheFnVI != null after this. |
| llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp | ||
|---|---|---|
| 961 ↗ | (On Diff #392291) | thanks for the suggestions! It's indeed due to internalization. I made the changes in https://reviews.llvm.org/D115492. |
| 961 ↗ | (On Diff #392291) | Thanks for the suggestion! Done in https://reviews.llvm.org/D115492 |