llvm-project/llvm/lib/Transforms/IPO/FunctionImport.cpp

Ignoring revisions in .git-blame-ignore-revs. Click here to bypass and see the normal blame view.

1367 lines
55 KiB
C++
Raw Normal View History

//===- FunctionImport.cpp - ThinLTO Summary-based Function Import ---------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
//===----------------------------------------------------------------------===//
//
// This file implements Function import based on summaries.
//
//===----------------------------------------------------------------------===//
#include "llvm/Transforms/IPO/FunctionImport.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/Statistic.h"
#include "llvm/ADT/StringMap.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/ADT/StringSet.h"
#include "llvm/Bitcode/BitcodeReader.h"
#include "llvm/IR/AutoUpgrade.h"
#include "llvm/IR/Constants.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/GlobalAlias.h"
#include "llvm/IR/GlobalObject.h"
#include "llvm/IR/GlobalValue.h"
#include "llvm/IR/GlobalVariable.h"
#include "llvm/IR/Metadata.h"
#include "llvm/IR/Module.h"
#include "llvm/IR/ModuleSummaryIndex.h"
#include "llvm/IRReader/IRReader.h"
#include "llvm/InitializePasses.h"
#include "llvm/Linker/IRMover.h"
#include "llvm/Object/ModuleSymbolTable.h"
#include "llvm/Object/SymbolicFile.h"
#include "llvm/Pass.h"
#include "llvm/Support/Casting.h"
#include "llvm/Support/CommandLine.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/Error.h"
#include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/SourceMgr.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/Transforms/IPO/Internalize.h"
#include "llvm/Transforms/Utils/Cloning.h"
#include "llvm/Transforms/Utils/FunctionImportUtils.h"
#include "llvm/Transforms/Utils/ValueMapper.h"
#include <cassert>
#include <memory>
#include <set>
#include <string>
#include <system_error>
#include <tuple>
#include <utility>
using namespace llvm;
#define DEBUG_TYPE "function-import"
STATISTIC(NumImportedFunctionsThinLink,
"Number of functions thin link decided to import");
STATISTIC(NumImportedHotFunctionsThinLink,
"Number of hot functions thin link decided to import");
STATISTIC(NumImportedCriticalFunctionsThinLink,
"Number of critical functions thin link decided to import");
STATISTIC(NumImportedGlobalVarsThinLink,
"Number of global variables thin link decided to import");
STATISTIC(NumImportedFunctions, "Number of functions imported in backend");
STATISTIC(NumImportedGlobalVars,
"Number of global variables imported in backend");
STATISTIC(NumImportedModules, "Number of modules imported from");
STATISTIC(NumDeadSymbols, "Number of dead stripped symbols in index");
STATISTIC(NumLiveSymbols, "Number of live symbols in index");
/// Limit on instruction count of imported functions.
static cl::opt<unsigned> ImportInstrLimit(
"import-instr-limit", cl::init(100), cl::Hidden, cl::value_desc("N"),
cl::desc("Only import functions with less than N instructions"));
static cl::opt<int> ImportCutoff(
"import-cutoff", cl::init(-1), cl::Hidden, cl::value_desc("N"),
cl::desc("Only import first N functions if N>=0 (default -1)"));
static cl::opt<float>
ImportInstrFactor("import-instr-evolution-factor", cl::init(0.7),
cl::Hidden, cl::value_desc("x"),
cl::desc("As we import functions, multiply the "
"`import-instr-limit` threshold by this factor "
"before processing newly imported functions"));
static cl::opt<float> ImportHotInstrFactor(
"import-hot-evolution-factor", cl::init(1.0), cl::Hidden,
cl::value_desc("x"),
cl::desc("As we import functions called from hot callsite, multiply the "
"`import-instr-limit` threshold by this factor "
"before processing newly imported functions"));
static cl::opt<float> ImportHotMultiplier(
"import-hot-multiplier", cl::init(10.0), cl::Hidden, cl::value_desc("x"),
cl::desc("Multiply the `import-instr-limit` threshold for hot callsites"));
static cl::opt<float> ImportCriticalMultiplier(
"import-critical-multiplier", cl::init(100.0), cl::Hidden,
cl::value_desc("x"),
cl::desc(
"Multiply the `import-instr-limit` threshold for critical callsites"));
// FIXME: This multiplier was not really tuned up.
static cl::opt<float> ImportColdMultiplier(
"import-cold-multiplier", cl::init(0), cl::Hidden, cl::value_desc("N"),
cl::desc("Multiply the `import-instr-limit` threshold for cold callsites"));
static cl::opt<bool> PrintImports("print-imports", cl::init(false), cl::Hidden,
cl::desc("Print imported functions"));
static cl::opt<bool> PrintImportFailures(
"print-import-failures", cl::init(false), cl::Hidden,
cl::desc("Print information for functions rejected for importing"));
static cl::opt<bool> ComputeDead("compute-dead", cl::init(true), cl::Hidden,
cl::desc("Compute dead symbols"));
static cl::opt<bool> EnableImportMetadata(
"enable-import-metadata", cl::init(
#if !defined(NDEBUG)
true /*Enabled with asserts.*/
#else
false
#endif
),
cl::Hidden, cl::desc("Enable import metadata like 'thinlto_src_module'"));
/// Summary file to use for function importing when using -function-import from
/// the command line.
static cl::opt<std::string>
SummaryFile("summary-file",
cl::desc("The summary file to use for function importing."));
/// Used when testing importing from distributed indexes via opt
// -function-import.
static cl::opt<bool>
ImportAllIndex("import-all-index",
cl::desc("Import all external functions in index."));
// Load lazily a module from \p FileName in \p Context.
static std::unique_ptr<Module> loadFile(const std::string &FileName,
LLVMContext &Context) {
SMDiagnostic Err;
LLVM_DEBUG(dbgs() << "Loading '" << FileName << "'\n");
// Metadata isn't loaded until functions are imported, to minimize
// the memory overhead.
std::unique_ptr<Module> Result =
getLazyIRFileModule(FileName, Err, Context,
/* ShouldLazyLoadMetadata = */ true);
if (!Result) {
Err.print("function-import", errs());
report_fatal_error("Abort");
}
return Result;
}
/// Given a list of possible callee implementation for a call site, select one
/// that fits the \p Threshold.
///
/// FIXME: select "best" instead of first that fits. But what is "best"?
/// - The smallest: more likely to be inlined.
/// - The one with the least outgoing edges (already well optimized).
/// - One from a module already being imported from in order to reduce the
/// number of source modules parsed/linked.
/// - One that has PGO data attached.
/// - [insert you fancy metric here]
static const GlobalValueSummary *
selectCallee(const ModuleSummaryIndex &Index,
ArrayRef<std::unique_ptr<GlobalValueSummary>> CalleeSummaryList,
unsigned Threshold, StringRef CallerModulePath,
FunctionImporter::ImportFailureReason &Reason,
GlobalValue::GUID GUID) {
Reason = FunctionImporter::ImportFailureReason::None;
auto It = llvm::find_if(
CalleeSummaryList,
[&](const std::unique_ptr<GlobalValueSummary> &SummaryPtr) {
auto *GVSummary = SummaryPtr.get();
if (!Index.isGlobalValueLive(GVSummary)) {
Reason = FunctionImporter::ImportFailureReason::NotLive;
return false;
}
// For SamplePGO, in computeImportForFunction the OriginalId
// may have been used to locate the callee summary list (See
// comment there).
// The mapping from OriginalId to GUID may return a GUID
// that corresponds to a static variable. Filter it out here.
// This can happen when
// 1) There is a call to a library function which is not defined
// in the index.
// 2) There is a static variable with the OriginalGUID identical
// to the GUID of the library function in 1);
// When this happens, the logic for SamplePGO kicks in and
// the static variable in 2) will be found, which needs to be
// filtered out.
if (GVSummary->getSummaryKind() == GlobalValueSummary::GlobalVarKind) {
Reason = FunctionImporter::ImportFailureReason::GlobalVar;
return false;
}
if (GlobalValue::isInterposableLinkage(GVSummary->linkage())) {
Reason = FunctionImporter::ImportFailureReason::InterposableLinkage;
// There is no point in importing these, we can't inline them
return false;
}
auto *Summary = cast<FunctionSummary>(GVSummary->getBaseObject());
// If this is a local function, make sure we import the copy
// in the caller's module. The only time a local function can
// share an entry in the index is if there is a local with the same name
// in another module that had the same source file name (in a different
// directory), where each was compiled in their own directory so there
// was not distinguishing path.
// However, do the import from another module if there is only one
// entry in the list - in that case this must be a reference due
// to indirect call profile data, since a function pointer can point to
// a local in another module.
if (GlobalValue::isLocalLinkage(Summary->linkage()) &&
CalleeSummaryList.size() > 1 &&
Summary->modulePath() != CallerModulePath) {
Reason =
FunctionImporter::ImportFailureReason::LocalLinkageNotInModule;
return false;
}
if ((Summary->instCount() > Threshold) &&
!Summary->fflags().AlwaysInline) {
Reason = FunctionImporter::ImportFailureReason::TooLarge;
return false;
}
// Skip if it isn't legal to import (e.g. may reference unpromotable
// locals).
if (Summary->notEligibleToImport()) {
Reason = FunctionImporter::ImportFailureReason::NotEligible;
return false;
}
// Don't bother importing if we can't inline it anyway.
if (Summary->fflags().NoInline) {
Reason = FunctionImporter::ImportFailureReason::NoInline;
return false;
}
return true;
});
if (It == CalleeSummaryList.end())
return nullptr;
return cast<GlobalValueSummary>(It->get());
}
namespace {
using EdgeInfo = std::tuple<const FunctionSummary *, unsigned /* Threshold */,
GlobalValue::GUID>;
} // anonymous namespace
static ValueInfo
updateValueInfoForIndirectCalls(const ModuleSummaryIndex &Index, ValueInfo VI) {
if (!VI.getSummaryList().empty())
return VI;
// For SamplePGO, the indirect call targets for local functions will
// have its original name annotated in profile. We try to find the
// corresponding PGOFuncName as the GUID.
// FIXME: Consider updating the edges in the graph after building
// it, rather than needing to perform this mapping on each walk.
auto GUID = Index.getGUIDFromOriginalID(VI.getGUID());
if (GUID == 0)
return ValueInfo();
return Index.getValueInfo(GUID);
}
static void computeImportForReferencedGlobals(
const FunctionSummary &Summary, const ModuleSummaryIndex &Index,
const GVSummaryMapTy &DefinedGVSummaries,
FunctionImporter::ImportMapTy &ImportList,
StringMap<FunctionImporter::ExportSetTy> *ExportLists) {
for (auto &VI : Summary.refs()) {
if (DefinedGVSummaries.count(VI.getGUID())) {
LLVM_DEBUG(
dbgs() << "Ref ignored! Target already in destination module.\n");
continue;
}
LLVM_DEBUG(dbgs() << " ref -> " << VI << "\n");
// If this is a local variable, make sure we import the copy
// in the caller's module. The only time a local variable can
// share an entry in the index is if there is a local with the same name
// in another module that had the same source file name (in a different
// directory), where each was compiled in their own directory so there
// was not distinguishing path.
auto LocalNotInModule = [&](const GlobalValueSummary *RefSummary) -> bool {
return GlobalValue::isLocalLinkage(RefSummary->linkage()) &&
RefSummary->modulePath() != Summary.modulePath();
};
for (auto &RefSummary : VI.getSummaryList())
if (isa<GlobalVarSummary>(RefSummary.get()) &&
Index.canImportGlobalVar(RefSummary.get(), /* AnalyzeRefs */ true) &&
!LocalNotInModule(RefSummary.get())) {
auto ILI = ImportList[RefSummary->modulePath()].insert(VI.getGUID());
[ThinLTO] More efficient export computation (NFC) Summary: A recent change to enable more importing of global variables with references exposed some efficiency issues with export computation. See D73724 for more information and detailed analysis. The first was specific to variable importing. The code was marking every copy of a referenced value (from possibly thousands of files in the case of linkonce_odr) as exported, and we only need to mark the copy in the module containing the variable def being imported as exported. The reason is that this is tracking what values are newly exported as a result of importing. Anything that was defined in another module and simply used in the exporting module is already exported, and would have been identified by the caller (e.g. the LTO API implementations). The second issue is that the code was re-adding previously exported values (along with all references). It is easy to identify when a variable was already imported into the same module (via the import list insert call return value), and we already did this for function importing. However, what we weren't doing for either function or variable importing was avoiding a re-insertion when it was previously exported into a different importing module. The reason we couldn't do this is there was no way of telling from the export list whether it was previously inserted there because its definition was exported (in which case we already marked all its references as exported) from when it was inserted there because it was referenced by another exported value (in which case we haven't yet inserted its own references). To address this we can restructure the way the export list is constructed. This patch only adds the actual imported definitions (variable or function) to the export list for its module during the import computation. After import computation is complete, where we were already post-processing the export list we go ahead and add all references made by those exported values to the export list. These changes speed up the thin link not only with constant variable importing enabled, but also without (due to the efficiency improvement in function importing). Some thin link user time measurements for one large application, average of 5 runs: With constant variable importing enabled: - without this patch: 479.5s - with this patch: 74.6s Without constant variable importing enabled: - without this patch: 80.6s - with this patch: 70.3s Note I have not re-enabled constant variable importing here, as I would like to do additional compile time measurements with these fixes first. Reviewers: evgeny777 Subscribers: mehdi_amini, inglorion, hiraditya, dexonsmith, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D73851
2020-02-03 00:06:43 +08:00
// Only update stat and exports if we haven't already imported this
// variable.
if (!ILI.second)
break;
NumImportedGlobalVarsThinLink++;
// Any references made by this variable will be marked exported later,
// in ComputeCrossModuleImport, after import decisions are complete,
// which is more efficient than adding them here.
if (ExportLists)
(*ExportLists)[RefSummary->modulePath()].insert(VI);
break;
}
}
}
static const char *
getFailureName(FunctionImporter::ImportFailureReason Reason) {
switch (Reason) {
case FunctionImporter::ImportFailureReason::None:
return "None";
case FunctionImporter::ImportFailureReason::GlobalVar:
return "GlobalVar";
case FunctionImporter::ImportFailureReason::NotLive:
return "NotLive";
case FunctionImporter::ImportFailureReason::TooLarge:
return "TooLarge";
case FunctionImporter::ImportFailureReason::InterposableLinkage:
return "InterposableLinkage";
case FunctionImporter::ImportFailureReason::LocalLinkageNotInModule:
return "LocalLinkageNotInModule";
case FunctionImporter::ImportFailureReason::NotEligible:
return "NotEligible";
case FunctionImporter::ImportFailureReason::NoInline:
return "NoInline";
}
llvm_unreachable("invalid reason");
}
/// Compute the list of functions to import for a given caller. Mark these
/// imported functions and the symbols they reference in their source module as
/// exported from their source module.
static void computeImportForFunction(
const FunctionSummary &Summary, const ModuleSummaryIndex &Index,
const unsigned Threshold, const GVSummaryMapTy &DefinedGVSummaries,
SmallVectorImpl<EdgeInfo> &Worklist,
FunctionImporter::ImportMapTy &ImportList,
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
StringMap<FunctionImporter::ExportSetTy> *ExportLists,
FunctionImporter::ImportThresholdsTy &ImportThresholds) {
computeImportForReferencedGlobals(Summary, Index, DefinedGVSummaries,
ImportList, ExportLists);
static int ImportCount = 0;
for (auto &Edge : Summary.calls()) {
ValueInfo VI = Edge.first;
LLVM_DEBUG(dbgs() << " edge -> " << VI << " Threshold:" << Threshold
<< "\n");
if (ImportCutoff >= 0 && ImportCount >= ImportCutoff) {
LLVM_DEBUG(dbgs() << "ignored! import-cutoff value of " << ImportCutoff
<< " reached.\n");
continue;
}
VI = updateValueInfoForIndirectCalls(Index, VI);
if (!VI)
continue;
SamplePGO ThinLTO ICP fix for local functions. Summary: In SamplePGO, if the profile is collected from non-LTO binary, and used to drive ThinLTO, the indirect call promotion may fail because ThinLTO adjusts local function names to avoid conflicts. There are two places of where the mismatch can happen: 1. thin-link prepends SourceFileName to front of FuncName to build the GUID (GlobalValue::getGlobalIdentifier). Unlike instrumentation FDO, SamplePGO does not use the PGOFuncName scheme and therefore the indirect call target profile data contains a hash of the OriginalName. 2. backend compiler promotes some local functions to global and appends .llvm.{$ModuleHash} to the end of the FuncName to derive PromotedFunctionName This patch tries at the best effort to find the GUID from the original local function name (in profile), and use that in ICP promotion, and in SamplePGO matching that happens in the backend after importing/inlining: 1. in thin-link, it builds the map from OriginalName to GUID so that when thin-link reads in indirect call target profile (represented by OriginalName), it knows which GUID to import. 2. in backend compiler, if sample profile reader cannot find a profile match for PromotedFunctionName, it will try to find if there is a match for OriginalFunctionName. 3. in backend compiler, we build symbol table entry for OriginalFunctionName and pointer to the same symbol of PromotedFunctionName, so that ICP can find the correct target to promote. Reviewers: mehdi_amini, tejohnson Reviewed By: tejohnson Subscribers: llvm-commits, Prazek Differential Revision: https://reviews.llvm.org/D30754 llvm-svn: 297757
2017-03-15 01:33:01 +08:00
if (DefinedGVSummaries.count(VI.getGUID())) {
LLVM_DEBUG(dbgs() << "ignored! Target already in destination module.\n");
continue;
}
auto GetBonusMultiplier = [](CalleeInfo::HotnessType Hotness) -> float {
if (Hotness == CalleeInfo::HotnessType::Hot)
return ImportHotMultiplier;
if (Hotness == CalleeInfo::HotnessType::Cold)
return ImportColdMultiplier;
if (Hotness == CalleeInfo::HotnessType::Critical)
return ImportCriticalMultiplier;
return 1.0;
};
const auto NewThreshold =
Threshold * GetBonusMultiplier(Edge.second.getHotness());
auto IT = ImportThresholds.insert(std::make_pair(
VI.getGUID(), std::make_tuple(NewThreshold, nullptr, nullptr)));
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
bool PreviouslyVisited = !IT.second;
auto &ProcessedThreshold = std::get<0>(IT.first->second);
auto &CalleeSummary = std::get<1>(IT.first->second);
auto &FailureInfo = std::get<2>(IT.first->second);
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
bool IsHotCallsite =
Edge.second.getHotness() == CalleeInfo::HotnessType::Hot;
bool IsCriticalCallsite =
Edge.second.getHotness() == CalleeInfo::HotnessType::Critical;
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
const FunctionSummary *ResolvedCalleeSummary = nullptr;
if (CalleeSummary) {
assert(PreviouslyVisited);
// Since the traversal of the call graph is DFS, we can revisit a function
// a second time with a higher threshold. In this case, it is added back
// to the worklist with the new threshold (so that its own callee chains
// can be considered with the higher threshold).
if (NewThreshold <= ProcessedThreshold) {
LLVM_DEBUG(
dbgs() << "ignored! Target was already imported with Threshold "
<< ProcessedThreshold << "\n");
continue;
}
// Update with new larger threshold.
ProcessedThreshold = NewThreshold;
ResolvedCalleeSummary = cast<FunctionSummary>(CalleeSummary);
} else {
// If we already rejected importing a callee at the same or higher
// threshold, don't waste time calling selectCallee.
if (PreviouslyVisited && NewThreshold <= ProcessedThreshold) {
LLVM_DEBUG(
dbgs() << "ignored! Target was already rejected with Threshold "
<< ProcessedThreshold << "\n");
if (PrintImportFailures) {
assert(FailureInfo &&
"Expected FailureInfo for previously rejected candidate");
FailureInfo->Attempts++;
}
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
continue;
}
ThinLTO: Don't import aliases of any kind (even linkonce_odr) Summary: Until a more advanced version of importing can be implemented for aliases (one that imports an alias as an available_externally definition of the aliasee), skip the narrow subset of cases that was possible but came at a cost: aliases of linkonce_odr functions could be imported because the linkonce_odr function could be safely duplicated from the source module. This came/comes at the cost of not being able to 'home' imported linkonce functions (they had to be emitted linkonce_odr in all the destination modules (even if they weren't used by an alias) rather than as available_externally - causing extra object size). Tangentially, this also was the only reason ThinLTO would emit multiple CUs in to the resulting DWARF - which happens to be a problem for Fission (there's a fix for this in GDB but not released yet, etc). (actually it's not the only reason - but I'm sending a patch to fix the other reason shortly) There's no reason to believe this particularly narrow alias importing was especially/meaningfully important, only that it was /possible/ to implement in this way. When a more general solution is done, it should still satisfy the DWARF concerns above, since the import will still be available_externally, and thus not create extra CUs. Since now all aliases are treated the same, I removed/simplified some test cases since they were testing corner cases where there are no longer any corners. Reviewers: tejohnson, mehdi_amini Differential Revision: https://reviews.llvm.org/D35875 llvm-svn: 309278
2017-07-27 23:09:06 +08:00
FunctionImporter::ImportFailureReason Reason;
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
CalleeSummary = selectCallee(Index, VI.getSummaryList(), NewThreshold,
Summary.modulePath(), Reason, VI.getGUID());
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
if (!CalleeSummary) {
// Update with new larger threshold if this was a retry (otherwise
// we would have already inserted with NewThreshold above). Also
// update failure info if requested.
if (PreviouslyVisited) {
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
ProcessedThreshold = NewThreshold;
if (PrintImportFailures) {
assert(FailureInfo &&
"Expected FailureInfo for previously rejected candidate");
FailureInfo->Reason = Reason;
FailureInfo->Attempts++;
FailureInfo->MaxHotness =
std::max(FailureInfo->MaxHotness, Edge.second.getHotness());
}
} else if (PrintImportFailures) {
assert(!FailureInfo &&
"Expected no FailureInfo for newly rejected candidate");
FailureInfo = std::make_unique<FunctionImporter::ImportFailureInfo>(
VI, Edge.second.getHotness(), Reason, 1);
}
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
LLVM_DEBUG(
dbgs() << "ignored! No qualifying callee with summary found.\n");
continue;
}
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
// "Resolve" the summary
CalleeSummary = CalleeSummary->getBaseObject();
ResolvedCalleeSummary = cast<FunctionSummary>(CalleeSummary);
assert((ResolvedCalleeSummary->fflags().AlwaysInline ||
(ResolvedCalleeSummary->instCount() <= NewThreshold)) &&
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
"selectCallee() didn't honor the threshold");
auto ExportModulePath = ResolvedCalleeSummary->modulePath();
auto ILI = ImportList[ExportModulePath].insert(VI.getGUID());
// We previously decided to import this GUID definition if it was already
// inserted in the set of imports from the exporting module.
bool PreviouslyImported = !ILI.second;
if (!PreviouslyImported) {
NumImportedFunctionsThinLink++;
if (IsHotCallsite)
NumImportedHotFunctionsThinLink++;
if (IsCriticalCallsite)
NumImportedCriticalFunctionsThinLink++;
}
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
[ThinLTO] More efficient export computation (NFC) Summary: A recent change to enable more importing of global variables with references exposed some efficiency issues with export computation. See D73724 for more information and detailed analysis. The first was specific to variable importing. The code was marking every copy of a referenced value (from possibly thousands of files in the case of linkonce_odr) as exported, and we only need to mark the copy in the module containing the variable def being imported as exported. The reason is that this is tracking what values are newly exported as a result of importing. Anything that was defined in another module and simply used in the exporting module is already exported, and would have been identified by the caller (e.g. the LTO API implementations). The second issue is that the code was re-adding previously exported values (along with all references). It is easy to identify when a variable was already imported into the same module (via the import list insert call return value), and we already did this for function importing. However, what we weren't doing for either function or variable importing was avoiding a re-insertion when it was previously exported into a different importing module. The reason we couldn't do this is there was no way of telling from the export list whether it was previously inserted there because its definition was exported (in which case we already marked all its references as exported) from when it was inserted there because it was referenced by another exported value (in which case we haven't yet inserted its own references). To address this we can restructure the way the export list is constructed. This patch only adds the actual imported definitions (variable or function) to the export list for its module during the import computation. After import computation is complete, where we were already post-processing the export list we go ahead and add all references made by those exported values to the export list. These changes speed up the thin link not only with constant variable importing enabled, but also without (due to the efficiency improvement in function importing). Some thin link user time measurements for one large application, average of 5 runs: With constant variable importing enabled: - without this patch: 479.5s - with this patch: 74.6s Without constant variable importing enabled: - without this patch: 80.6s - with this patch: 70.3s Note I have not re-enabled constant variable importing here, as I would like to do additional compile time measurements with these fixes first. Reviewers: evgeny777 Subscribers: mehdi_amini, inglorion, hiraditya, dexonsmith, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D73851
2020-02-03 00:06:43 +08:00
// Any calls/references made by this function will be marked exported
// later, in ComputeCrossModuleImport, after import decisions are
// complete, which is more efficient than adding them here.
if (ExportLists)
(*ExportLists)[ExportModulePath].insert(VI);
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
}
auto GetAdjustedThreshold = [](unsigned Threshold, bool IsHotCallsite) {
// Adjust the threshold for next level of imported functions.
// The threshold is different for hot callsites because we can then
// inline chains of hot calls.
if (IsHotCallsite)
return Threshold * ImportHotInstrFactor;
return Threshold * ImportInstrFactor;
};
const auto AdjThreshold = GetAdjustedThreshold(Threshold, IsHotCallsite);
ImportCount++;
// Insert the newly imported function to the worklist.
Worklist.emplace_back(ResolvedCalleeSummary, AdjThreshold, VI.getGUID());
}
}
/// Given the list of globals defined in a module, compute the list of imports
/// as well as the list of "exports", i.e. the list of symbols referenced from
/// another module (that may require promotion).
static void ComputeImportForModule(
const GVSummaryMapTy &DefinedGVSummaries, const ModuleSummaryIndex &Index,
StringRef ModName, FunctionImporter::ImportMapTy &ImportList,
StringMap<FunctionImporter::ExportSetTy> *ExportLists = nullptr) {
// Worklist contains the list of function imported in this module, for which
// we will analyse the callees and may import further down the callgraph.
SmallVector<EdgeInfo, 128> Worklist;
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
FunctionImporter::ImportThresholdsTy ImportThresholds;
// Populate the worklist with the import for the functions in the current
// module
for (auto &GVSummary : DefinedGVSummaries) {
#ifndef NDEBUG
// FIXME: Change the GVSummaryMapTy to hold ValueInfo instead of GUID
// so this map look up (and possibly others) can be avoided.
auto VI = Index.getValueInfo(GVSummary.first);
#endif
if (!Index.isGlobalValueLive(GVSummary.second)) {
LLVM_DEBUG(dbgs() << "Ignores Dead GUID: " << VI << "\n");
continue;
}
auto *FuncSummary =
dyn_cast<FunctionSummary>(GVSummary.second->getBaseObject());
if (!FuncSummary)
// Skip import for global variables
continue;
LLVM_DEBUG(dbgs() << "Initialize import for " << VI << "\n");
computeImportForFunction(*FuncSummary, Index, ImportInstrLimit,
DefinedGVSummaries, Worklist, ImportList,
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
ExportLists, ImportThresholds);
}
// Process the newly imported functions and add callees to the worklist.
while (!Worklist.empty()) {
auto FuncInfo = Worklist.pop_back_val();
auto *Summary = std::get<0>(FuncInfo);
auto Threshold = std::get<1>(FuncInfo);
computeImportForFunction(*Summary, Index, Threshold, DefinedGVSummaries,
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
Worklist, ImportList, ExportLists,
ImportThresholds);
}
// Print stats about functions considered but rejected for importing
// when requested.
if (PrintImportFailures) {
dbgs() << "Missed imports into module " << ModName << "\n";
for (auto &I : ImportThresholds) {
auto &ProcessedThreshold = std::get<0>(I.second);
auto &CalleeSummary = std::get<1>(I.second);
auto &FailureInfo = std::get<2>(I.second);
if (CalleeSummary)
continue; // We are going to import.
assert(FailureInfo);
FunctionSummary *FS = nullptr;
if (!FailureInfo->VI.getSummaryList().empty())
FS = dyn_cast<FunctionSummary>(
FailureInfo->VI.getSummaryList()[0]->getBaseObject());
dbgs() << FailureInfo->VI
<< ": Reason = " << getFailureName(FailureInfo->Reason)
<< ", Threshold = " << ProcessedThreshold
<< ", Size = " << (FS ? (int)FS->instCount() : -1)
<< ", MaxHotness = " << getHotnessName(FailureInfo->MaxHotness)
<< ", Attempts = " << FailureInfo->Attempts << "\n";
}
}
}
#ifndef NDEBUG
static bool isGlobalVarSummary(const ModuleSummaryIndex &Index, ValueInfo VI) {
auto SL = VI.getSummaryList();
return SL.empty()
? false
: SL[0]->getSummaryKind() == GlobalValueSummary::GlobalVarKind;
}
static bool isGlobalVarSummary(const ModuleSummaryIndex &Index,
GlobalValue::GUID G) {
if (const auto &VI = Index.getValueInfo(G))
return isGlobalVarSummary(Index, VI);
return false;
}
template <class T>
static unsigned numGlobalVarSummaries(const ModuleSummaryIndex &Index,
T &Cont) {
unsigned NumGVS = 0;
for (auto &V : Cont)
if (isGlobalVarSummary(Index, V))
++NumGVS;
return NumGVS;
}
#endif
#ifndef NDEBUG
static bool
checkVariableImport(const ModuleSummaryIndex &Index,
StringMap<FunctionImporter::ImportMapTy> &ImportLists,
StringMap<FunctionImporter::ExportSetTy> &ExportLists) {
DenseSet<GlobalValue::GUID> FlattenedImports;
for (auto &ImportPerModule : ImportLists)
for (auto &ExportPerModule : ImportPerModule.second)
FlattenedImports.insert(ExportPerModule.second.begin(),
ExportPerModule.second.end());
// Checks that all GUIDs of read/writeonly vars we see in export lists
// are also in the import lists. Otherwise we my face linker undefs,
// because readonly and writeonly vars are internalized in their
// source modules.
auto IsReadOrWriteOnlyVar = [&](StringRef ModulePath, const ValueInfo &VI) {
auto *GVS = dyn_cast_or_null<GlobalVarSummary>(
Index.findSummaryInModule(VI, ModulePath));
return GVS && (Index.isReadOnly(GVS) || Index.isWriteOnly(GVS));
};
for (auto &ExportPerModule : ExportLists)
for (auto &VI : ExportPerModule.second)
if (!FlattenedImports.count(VI.getGUID()) &&
IsReadOrWriteOnlyVar(ExportPerModule.first(), VI))
return false;
return true;
}
#endif
/// Compute all the import and export for every module using the Index.
void llvm::ComputeCrossModuleImport(
const ModuleSummaryIndex &Index,
const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
StringMap<FunctionImporter::ImportMapTy> &ImportLists,
StringMap<FunctionImporter::ExportSetTy> &ExportLists) {
// For each module that has function defined, compute the import/export lists.
for (auto &DefinedGVSummaries : ModuleToDefinedGVSummaries) {
auto &ImportList = ImportLists[DefinedGVSummaries.first()];
LLVM_DEBUG(dbgs() << "Computing import for Module '"
<< DefinedGVSummaries.first() << "'\n");
ComputeImportForModule(DefinedGVSummaries.second, Index,
DefinedGVSummaries.first(), ImportList,
&ExportLists);
}
[ThinLTO] More efficient export computation (NFC) Summary: A recent change to enable more importing of global variables with references exposed some efficiency issues with export computation. See D73724 for more information and detailed analysis. The first was specific to variable importing. The code was marking every copy of a referenced value (from possibly thousands of files in the case of linkonce_odr) as exported, and we only need to mark the copy in the module containing the variable def being imported as exported. The reason is that this is tracking what values are newly exported as a result of importing. Anything that was defined in another module and simply used in the exporting module is already exported, and would have been identified by the caller (e.g. the LTO API implementations). The second issue is that the code was re-adding previously exported values (along with all references). It is easy to identify when a variable was already imported into the same module (via the import list insert call return value), and we already did this for function importing. However, what we weren't doing for either function or variable importing was avoiding a re-insertion when it was previously exported into a different importing module. The reason we couldn't do this is there was no way of telling from the export list whether it was previously inserted there because its definition was exported (in which case we already marked all its references as exported) from when it was inserted there because it was referenced by another exported value (in which case we haven't yet inserted its own references). To address this we can restructure the way the export list is constructed. This patch only adds the actual imported definitions (variable or function) to the export list for its module during the import computation. After import computation is complete, where we were already post-processing the export list we go ahead and add all references made by those exported values to the export list. These changes speed up the thin link not only with constant variable importing enabled, but also without (due to the efficiency improvement in function importing). Some thin link user time measurements for one large application, average of 5 runs: With constant variable importing enabled: - without this patch: 479.5s - with this patch: 74.6s Without constant variable importing enabled: - without this patch: 80.6s - with this patch: 70.3s Note I have not re-enabled constant variable importing here, as I would like to do additional compile time measurements with these fixes first. Reviewers: evgeny777 Subscribers: mehdi_amini, inglorion, hiraditya, dexonsmith, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D73851
2020-02-03 00:06:43 +08:00
// When computing imports we only added the variables and functions being
// imported to the export list. We also need to mark any references and calls
// they make as exported as well. We do this here, as it is more efficient
// since we may import the same values multiple times into different modules
// during the import computation.
for (auto &ELI : ExportLists) {
[ThinLTO] More efficient export computation (NFC) Summary: A recent change to enable more importing of global variables with references exposed some efficiency issues with export computation. See D73724 for more information and detailed analysis. The first was specific to variable importing. The code was marking every copy of a referenced value (from possibly thousands of files in the case of linkonce_odr) as exported, and we only need to mark the copy in the module containing the variable def being imported as exported. The reason is that this is tracking what values are newly exported as a result of importing. Anything that was defined in another module and simply used in the exporting module is already exported, and would have been identified by the caller (e.g. the LTO API implementations). The second issue is that the code was re-adding previously exported values (along with all references). It is easy to identify when a variable was already imported into the same module (via the import list insert call return value), and we already did this for function importing. However, what we weren't doing for either function or variable importing was avoiding a re-insertion when it was previously exported into a different importing module. The reason we couldn't do this is there was no way of telling from the export list whether it was previously inserted there because its definition was exported (in which case we already marked all its references as exported) from when it was inserted there because it was referenced by another exported value (in which case we haven't yet inserted its own references). To address this we can restructure the way the export list is constructed. This patch only adds the actual imported definitions (variable or function) to the export list for its module during the import computation. After import computation is complete, where we were already post-processing the export list we go ahead and add all references made by those exported values to the export list. These changes speed up the thin link not only with constant variable importing enabled, but also without (due to the efficiency improvement in function importing). Some thin link user time measurements for one large application, average of 5 runs: With constant variable importing enabled: - without this patch: 479.5s - with this patch: 74.6s Without constant variable importing enabled: - without this patch: 80.6s - with this patch: 70.3s Note I have not re-enabled constant variable importing here, as I would like to do additional compile time measurements with these fixes first. Reviewers: evgeny777 Subscribers: mehdi_amini, inglorion, hiraditya, dexonsmith, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D73851
2020-02-03 00:06:43 +08:00
FunctionImporter::ExportSetTy NewExports;
const auto &DefinedGVSummaries =
ModuleToDefinedGVSummaries.lookup(ELI.first());
[ThinLTO] More efficient export computation (NFC) Summary: A recent change to enable more importing of global variables with references exposed some efficiency issues with export computation. See D73724 for more information and detailed analysis. The first was specific to variable importing. The code was marking every copy of a referenced value (from possibly thousands of files in the case of linkonce_odr) as exported, and we only need to mark the copy in the module containing the variable def being imported as exported. The reason is that this is tracking what values are newly exported as a result of importing. Anything that was defined in another module and simply used in the exporting module is already exported, and would have been identified by the caller (e.g. the LTO API implementations). The second issue is that the code was re-adding previously exported values (along with all references). It is easy to identify when a variable was already imported into the same module (via the import list insert call return value), and we already did this for function importing. However, what we weren't doing for either function or variable importing was avoiding a re-insertion when it was previously exported into a different importing module. The reason we couldn't do this is there was no way of telling from the export list whether it was previously inserted there because its definition was exported (in which case we already marked all its references as exported) from when it was inserted there because it was referenced by another exported value (in which case we haven't yet inserted its own references). To address this we can restructure the way the export list is constructed. This patch only adds the actual imported definitions (variable or function) to the export list for its module during the import computation. After import computation is complete, where we were already post-processing the export list we go ahead and add all references made by those exported values to the export list. These changes speed up the thin link not only with constant variable importing enabled, but also without (due to the efficiency improvement in function importing). Some thin link user time measurements for one large application, average of 5 runs: With constant variable importing enabled: - without this patch: 479.5s - with this patch: 74.6s Without constant variable importing enabled: - without this patch: 80.6s - with this patch: 70.3s Note I have not re-enabled constant variable importing here, as I would like to do additional compile time measurements with these fixes first. Reviewers: evgeny777 Subscribers: mehdi_amini, inglorion, hiraditya, dexonsmith, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D73851
2020-02-03 00:06:43 +08:00
for (auto &EI : ELI.second) {
// Find the copy defined in the exporting module so that we can mark the
// values it references in that specific definition as exported.
// Below we will add all references and called values, without regard to
// whether they are also defined in this module. We subsequently prune the
// list to only include those defined in the exporting module, see comment
// there as to why.
auto DS = DefinedGVSummaries.find(EI.getGUID());
// Anything marked exported during the import computation must have been
// defined in the exporting module.
assert(DS != DefinedGVSummaries.end());
auto *S = DS->getSecond();
S = S->getBaseObject();
if (auto *GVS = dyn_cast<GlobalVarSummary>(S)) {
// Export referenced functions and variables. We don't export/promote
// objects referenced by writeonly variable initializer, because
// we convert such variables initializers to "zeroinitializer".
// See processGlobalForThinLTO.
if (!Index.isWriteOnly(GVS))
for (const auto &VI : GVS->refs())
NewExports.insert(VI);
} else {
auto *FS = cast<FunctionSummary>(S);
for (auto &Edge : FS->calls())
NewExports.insert(Edge.first);
for (auto &Ref : FS->refs())
NewExports.insert(Ref);
}
}
// Prune list computed above to only include values defined in the exporting
// module. We do this after the above insertion since we may hit the same
// ref/call target multiple times in above loop, and it is more efficient to
// avoid a set lookup each time.
for (auto EI = NewExports.begin(); EI != NewExports.end();) {
if (!DefinedGVSummaries.count(EI->getGUID()))
[ThinLTO] More efficient export computation (NFC) Summary: A recent change to enable more importing of global variables with references exposed some efficiency issues with export computation. See D73724 for more information and detailed analysis. The first was specific to variable importing. The code was marking every copy of a referenced value (from possibly thousands of files in the case of linkonce_odr) as exported, and we only need to mark the copy in the module containing the variable def being imported as exported. The reason is that this is tracking what values are newly exported as a result of importing. Anything that was defined in another module and simply used in the exporting module is already exported, and would have been identified by the caller (e.g. the LTO API implementations). The second issue is that the code was re-adding previously exported values (along with all references). It is easy to identify when a variable was already imported into the same module (via the import list insert call return value), and we already did this for function importing. However, what we weren't doing for either function or variable importing was avoiding a re-insertion when it was previously exported into a different importing module. The reason we couldn't do this is there was no way of telling from the export list whether it was previously inserted there because its definition was exported (in which case we already marked all its references as exported) from when it was inserted there because it was referenced by another exported value (in which case we haven't yet inserted its own references). To address this we can restructure the way the export list is constructed. This patch only adds the actual imported definitions (variable or function) to the export list for its module during the import computation. After import computation is complete, where we were already post-processing the export list we go ahead and add all references made by those exported values to the export list. These changes speed up the thin link not only with constant variable importing enabled, but also without (due to the efficiency improvement in function importing). Some thin link user time measurements for one large application, average of 5 runs: With constant variable importing enabled: - without this patch: 479.5s - with this patch: 74.6s Without constant variable importing enabled: - without this patch: 80.6s - with this patch: 70.3s Note I have not re-enabled constant variable importing here, as I would like to do additional compile time measurements with these fixes first. Reviewers: evgeny777 Subscribers: mehdi_amini, inglorion, hiraditya, dexonsmith, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D73851
2020-02-03 00:06:43 +08:00
NewExports.erase(EI++);
else
++EI;
}
[ThinLTO] More efficient export computation (NFC) Summary: A recent change to enable more importing of global variables with references exposed some efficiency issues with export computation. See D73724 for more information and detailed analysis. The first was specific to variable importing. The code was marking every copy of a referenced value (from possibly thousands of files in the case of linkonce_odr) as exported, and we only need to mark the copy in the module containing the variable def being imported as exported. The reason is that this is tracking what values are newly exported as a result of importing. Anything that was defined in another module and simply used in the exporting module is already exported, and would have been identified by the caller (e.g. the LTO API implementations). The second issue is that the code was re-adding previously exported values (along with all references). It is easy to identify when a variable was already imported into the same module (via the import list insert call return value), and we already did this for function importing. However, what we weren't doing for either function or variable importing was avoiding a re-insertion when it was previously exported into a different importing module. The reason we couldn't do this is there was no way of telling from the export list whether it was previously inserted there because its definition was exported (in which case we already marked all its references as exported) from when it was inserted there because it was referenced by another exported value (in which case we haven't yet inserted its own references). To address this we can restructure the way the export list is constructed. This patch only adds the actual imported definitions (variable or function) to the export list for its module during the import computation. After import computation is complete, where we were already post-processing the export list we go ahead and add all references made by those exported values to the export list. These changes speed up the thin link not only with constant variable importing enabled, but also without (due to the efficiency improvement in function importing). Some thin link user time measurements for one large application, average of 5 runs: With constant variable importing enabled: - without this patch: 479.5s - with this patch: 74.6s Without constant variable importing enabled: - without this patch: 80.6s - with this patch: 70.3s Note I have not re-enabled constant variable importing here, as I would like to do additional compile time measurements with these fixes first. Reviewers: evgeny777 Subscribers: mehdi_amini, inglorion, hiraditya, dexonsmith, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D73851
2020-02-03 00:06:43 +08:00
ELI.second.insert(NewExports.begin(), NewExports.end());
}
assert(checkVariableImport(Index, ImportLists, ExportLists));
#ifndef NDEBUG
LLVM_DEBUG(dbgs() << "Import/Export lists for " << ImportLists.size()
<< " modules:\n");
for (auto &ModuleImports : ImportLists) {
auto ModName = ModuleImports.first();
auto &Exports = ExportLists[ModName];
unsigned NumGVS = numGlobalVarSummaries(Index, Exports);
LLVM_DEBUG(dbgs() << "* Module " << ModName << " exports "
<< Exports.size() - NumGVS << " functions and " << NumGVS
<< " vars. Imports from " << ModuleImports.second.size()
<< " modules.\n");
for (auto &Src : ModuleImports.second) {
auto SrcModName = Src.first();
unsigned NumGVSPerMod = numGlobalVarSummaries(Index, Src.second);
LLVM_DEBUG(dbgs() << " - " << Src.second.size() - NumGVSPerMod
<< " functions imported from " << SrcModName << "\n");
LLVM_DEBUG(dbgs() << " - " << NumGVSPerMod
<< " global vars imported from " << SrcModName << "\n");
}
}
#endif
}
#ifndef NDEBUG
static void dumpImportListForModule(const ModuleSummaryIndex &Index,
StringRef ModulePath,
FunctionImporter::ImportMapTy &ImportList) {
LLVM_DEBUG(dbgs() << "* Module " << ModulePath << " imports from "
<< ImportList.size() << " modules.\n");
for (auto &Src : ImportList) {
auto SrcModName = Src.first();
unsigned NumGVSPerMod = numGlobalVarSummaries(Index, Src.second);
LLVM_DEBUG(dbgs() << " - " << Src.second.size() - NumGVSPerMod
<< " functions imported from " << SrcModName << "\n");
LLVM_DEBUG(dbgs() << " - " << NumGVSPerMod << " vars imported from "
<< SrcModName << "\n");
}
}
#endif
/// Compute all the imports for the given module in the Index.
void llvm::ComputeCrossModuleImportForModule(
StringRef ModulePath, const ModuleSummaryIndex &Index,
FunctionImporter::ImportMapTy &ImportList) {
// Collect the list of functions this module defines.
// GUID -> Summary
GVSummaryMapTy FunctionSummaryMap;
Index.collectDefinedFunctionsForModule(ModulePath, FunctionSummaryMap);
// Compute the import list for this module.
LLVM_DEBUG(dbgs() << "Computing import for Module '" << ModulePath << "'\n");
ComputeImportForModule(FunctionSummaryMap, Index, ModulePath, ImportList);
#ifndef NDEBUG
dumpImportListForModule(Index, ModulePath, ImportList);
#endif
}
// Mark all external summaries in Index for import into the given module.
// Used for distributed builds using a distributed index.
void llvm::ComputeCrossModuleImportForModuleFromIndex(
StringRef ModulePath, const ModuleSummaryIndex &Index,
FunctionImporter::ImportMapTy &ImportList) {
for (auto &GlobalList : Index) {
// Ignore entries for undefined references.
if (GlobalList.second.SummaryList.empty())
continue;
auto GUID = GlobalList.first;
assert(GlobalList.second.SummaryList.size() == 1 &&
"Expected individual combined index to have one summary per GUID");
auto &Summary = GlobalList.second.SummaryList[0];
// Skip the summaries for the importing module. These are included to
// e.g. record required linkage changes.
if (Summary->modulePath() == ModulePath)
continue;
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
// Add an entry to provoke importing by thinBackend.
ImportList[Summary->modulePath()].insert(GUID);
}
#ifndef NDEBUG
dumpImportListForModule(Index, ModulePath, ImportList);
#endif
}
void llvm::computeDeadSymbols(
ModuleSummaryIndex &Index,
const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols,
function_ref<PrevailingType(GlobalValue::GUID)> isPrevailing) {
assert(!Index.withGlobalValueDeadStripping());
if (!ComputeDead)
return;
if (GUIDPreservedSymbols.empty())
// Don't do anything when nothing is live, this is friendly with tests.
return;
unsigned LiveSymbols = 0;
SmallVector<ValueInfo, 128> Worklist;
Worklist.reserve(GUIDPreservedSymbols.size() * 2);
for (auto GUID : GUIDPreservedSymbols) {
ValueInfo VI = Index.getValueInfo(GUID);
if (!VI)
continue;
for (auto &S : VI.getSummaryList())
S->setLive(true);
}
// Add values flagged in the index as live roots to the worklist.
for (const auto &Entry : Index) {
auto VI = Index.getValueInfo(Entry);
for (auto &S : Entry.second.SummaryList)
if (S->isLive()) {
LLVM_DEBUG(dbgs() << "Live root: " << VI << "\n");
Worklist.push_back(VI);
++LiveSymbols;
break;
}
}
// Make value live and add it to the worklist if it was not live before.
[ThinLTO] Fix handling of weak interposable symbols Summary: Keep aliasees alive if their alias is live, otherwise we end up with an alias to a declaration, which is invalid. This can happen when the aliasee is weak and non-prevailing. This fix exposed the fact that we were then attempting to internalize the weak symbol, which was not exported as it was not prevailing. We should not internalize interposable symbols in general, unless this is the prevailing copy, since it can lead to incorrect inlining and other optimizations. Most of the changes in this patch are due to the restructuring required to pass down the prevailing callback. Finally, while implementing the test cases, I found that in the case of a weak aliasee that is still marked not live because its alias isn't live, after dropping the definition we incorrectly marked the declaration with weak linkage when resolving prevailing symbols in the module. This was due to some special case handling for symbols marked WeakLinkage in the summary located before instead of after a subsequent check for the symbol being a declaration. It turns out that we don't actually need this special case handling any more (looking back at the history, when that was added the code was structured quite differently) - we will correctly mark with weak linkage further below when the definition hasn't been dropped. Fixes PR42542. Reviewers: pcc Subscribers: mehdi_amini, inglorion, steven_wu, dexonsmith, dang, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D66264 llvm-svn: 369766
2019-08-23 23:18:58 +08:00
auto visit = [&](ValueInfo VI, bool IsAliasee) {
// FIXME: If we knew which edges were created for indirect call profiles,
// we could skip them here. Any that are live should be reached via
// other edges, e.g. reference edges. Otherwise, using a profile collected
// on a slightly different binary might provoke preserving, importing
// and ultimately promoting calls to functions not linked into this
// binary, which increases the binary size unnecessarily. Note that
// if this code changes, the importer needs to change so that edges
// to functions marked dead are skipped.
VI = updateValueInfoForIndirectCalls(Index, VI);
if (!VI)
return;
[ThinLTO] Refine reachability check to fix compile time increase Summary: A recent fix to the ThinLTO whole program dead code elimination (D56117) increased the thin link time on a large MSAN'ed binary by 2x. It's likely that the time increased elsewhere, but was more noticeable here since it was already large and ended up timing out. That change made it so we would repeatedly scan all copies of linkonce symbols for liveness every time they were encountered during the graph traversal. This was needed since we only mark one copy of an aliasee as live when we encounter a live alias. This patch fixes the issue in a more efficient manner by simply proactively visiting the aliasee (thus marking all copies live) when we encounter a live alias. Two notes: One, this requires a hash table lookup (finding the aliasee summary in the index based on aliasee GUID). However, the impact of this seems to be small compared to the original pre-D56117 thin link time. It could be addressed if we keep the aliasee ValueInfo in the alias summary instead of the aliasee GUID, which I am exploring in a separate patch. Second, we only populate the aliasee GUID field when reading summaries from bitcode (whether we are reading individual summaries and merging on the fly to form the compiled index, or reading in a serialized combined index). Thankfully, that's currently the only way we can get to this code as we don't yet support reading summaries from LLVM assembly directly into a tool that performs the thin link (they must be converted to bitcode first). I added a FIXME, however I have the fix under test already. The easiest fix is to simply populate this field always, which isn't hard, but more likely the change I am exploring to store the ValueInfo instead as described above will subsume this. I don't want to hold up the regression fix for this though. Reviewers: trentxintong Subscribers: mehdi_amini, inglorion, dexonsmith, llvm-commits Differential Revision: https://reviews.llvm.org/D57203 llvm-svn: 352438
2019-01-29 06:27:05 +08:00
if (llvm::any_of(VI.getSummaryList(),
[](const std::unique_ptr<llvm::GlobalValueSummary> &S) {
return S->isLive();
}))
return;
// We only keep live symbols that are known to be non-prevailing if any are
// available_externally, linkonceodr, weakodr. Those symbols are discarded
// later in the EliminateAvailableExternally pass and setting them to
// not-live could break downstreams users of liveness information (PR36483)
// or limit optimization opportunities.
if (isPrevailing(VI.getGUID()) == PrevailingType::No) {
bool KeepAliveLinkage = false;
bool Interposable = false;
for (auto &S : VI.getSummaryList()) {
if (S->linkage() == GlobalValue::AvailableExternallyLinkage ||
S->linkage() == GlobalValue::WeakODRLinkage ||
S->linkage() == GlobalValue::LinkOnceODRLinkage)
KeepAliveLinkage = true;
else if (GlobalValue::isInterposableLinkage(S->linkage()))
Interposable = true;
}
[ThinLTO] Fix handling of weak interposable symbols Summary: Keep aliasees alive if their alias is live, otherwise we end up with an alias to a declaration, which is invalid. This can happen when the aliasee is weak and non-prevailing. This fix exposed the fact that we were then attempting to internalize the weak symbol, which was not exported as it was not prevailing. We should not internalize interposable symbols in general, unless this is the prevailing copy, since it can lead to incorrect inlining and other optimizations. Most of the changes in this patch are due to the restructuring required to pass down the prevailing callback. Finally, while implementing the test cases, I found that in the case of a weak aliasee that is still marked not live because its alias isn't live, after dropping the definition we incorrectly marked the declaration with weak linkage when resolving prevailing symbols in the module. This was due to some special case handling for symbols marked WeakLinkage in the summary located before instead of after a subsequent check for the symbol being a declaration. It turns out that we don't actually need this special case handling any more (looking back at the history, when that was added the code was structured quite differently) - we will correctly mark with weak linkage further below when the definition hasn't been dropped. Fixes PR42542. Reviewers: pcc Subscribers: mehdi_amini, inglorion, steven_wu, dexonsmith, dang, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D66264 llvm-svn: 369766
2019-08-23 23:18:58 +08:00
if (!IsAliasee) {
if (!KeepAliveLinkage)
return;
[ThinLTO] Fix handling of weak interposable symbols Summary: Keep aliasees alive if their alias is live, otherwise we end up with an alias to a declaration, which is invalid. This can happen when the aliasee is weak and non-prevailing. This fix exposed the fact that we were then attempting to internalize the weak symbol, which was not exported as it was not prevailing. We should not internalize interposable symbols in general, unless this is the prevailing copy, since it can lead to incorrect inlining and other optimizations. Most of the changes in this patch are due to the restructuring required to pass down the prevailing callback. Finally, while implementing the test cases, I found that in the case of a weak aliasee that is still marked not live because its alias isn't live, after dropping the definition we incorrectly marked the declaration with weak linkage when resolving prevailing symbols in the module. This was due to some special case handling for symbols marked WeakLinkage in the summary located before instead of after a subsequent check for the symbol being a declaration. It turns out that we don't actually need this special case handling any more (looking back at the history, when that was added the code was structured quite differently) - we will correctly mark with weak linkage further below when the definition hasn't been dropped. Fixes PR42542. Reviewers: pcc Subscribers: mehdi_amini, inglorion, steven_wu, dexonsmith, dang, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D66264 llvm-svn: 369766
2019-08-23 23:18:58 +08:00
if (Interposable)
report_fatal_error(
"Interposable and available_externally/linkonce_odr/weak_odr "
"symbol");
}
}
for (auto &S : VI.getSummaryList())
S->setLive(true);
++LiveSymbols;
Worklist.push_back(VI);
};
while (!Worklist.empty()) {
auto VI = Worklist.pop_back_val();
for (auto &Summary : VI.getSummaryList()) {
[ThinLTO] Refine reachability check to fix compile time increase Summary: A recent fix to the ThinLTO whole program dead code elimination (D56117) increased the thin link time on a large MSAN'ed binary by 2x. It's likely that the time increased elsewhere, but was more noticeable here since it was already large and ended up timing out. That change made it so we would repeatedly scan all copies of linkonce symbols for liveness every time they were encountered during the graph traversal. This was needed since we only mark one copy of an aliasee as live when we encounter a live alias. This patch fixes the issue in a more efficient manner by simply proactively visiting the aliasee (thus marking all copies live) when we encounter a live alias. Two notes: One, this requires a hash table lookup (finding the aliasee summary in the index based on aliasee GUID). However, the impact of this seems to be small compared to the original pre-D56117 thin link time. It could be addressed if we keep the aliasee ValueInfo in the alias summary instead of the aliasee GUID, which I am exploring in a separate patch. Second, we only populate the aliasee GUID field when reading summaries from bitcode (whether we are reading individual summaries and merging on the fly to form the compiled index, or reading in a serialized combined index). Thankfully, that's currently the only way we can get to this code as we don't yet support reading summaries from LLVM assembly directly into a tool that performs the thin link (they must be converted to bitcode first). I added a FIXME, however I have the fix under test already. The easiest fix is to simply populate this field always, which isn't hard, but more likely the change I am exploring to store the ValueInfo instead as described above will subsume this. I don't want to hold up the regression fix for this though. Reviewers: trentxintong Subscribers: mehdi_amini, inglorion, dexonsmith, llvm-commits Differential Revision: https://reviews.llvm.org/D57203 llvm-svn: 352438
2019-01-29 06:27:05 +08:00
if (auto *AS = dyn_cast<AliasSummary>(Summary.get())) {
// If this is an alias, visit the aliasee VI to ensure that all copies
// are marked live and it is added to the worklist for further
// processing of its references.
[ThinLTO] Fix handling of weak interposable symbols Summary: Keep aliasees alive if their alias is live, otherwise we end up with an alias to a declaration, which is invalid. This can happen when the aliasee is weak and non-prevailing. This fix exposed the fact that we were then attempting to internalize the weak symbol, which was not exported as it was not prevailing. We should not internalize interposable symbols in general, unless this is the prevailing copy, since it can lead to incorrect inlining and other optimizations. Most of the changes in this patch are due to the restructuring required to pass down the prevailing callback. Finally, while implementing the test cases, I found that in the case of a weak aliasee that is still marked not live because its alias isn't live, after dropping the definition we incorrectly marked the declaration with weak linkage when resolving prevailing symbols in the module. This was due to some special case handling for symbols marked WeakLinkage in the summary located before instead of after a subsequent check for the symbol being a declaration. It turns out that we don't actually need this special case handling any more (looking back at the history, when that was added the code was structured quite differently) - we will correctly mark with weak linkage further below when the definition hasn't been dropped. Fixes PR42542. Reviewers: pcc Subscribers: mehdi_amini, inglorion, steven_wu, dexonsmith, dang, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D66264 llvm-svn: 369766
2019-08-23 23:18:58 +08:00
visit(AS->getAliaseeVI(), true);
[ThinLTO] Refine reachability check to fix compile time increase Summary: A recent fix to the ThinLTO whole program dead code elimination (D56117) increased the thin link time on a large MSAN'ed binary by 2x. It's likely that the time increased elsewhere, but was more noticeable here since it was already large and ended up timing out. That change made it so we would repeatedly scan all copies of linkonce symbols for liveness every time they were encountered during the graph traversal. This was needed since we only mark one copy of an aliasee as live when we encounter a live alias. This patch fixes the issue in a more efficient manner by simply proactively visiting the aliasee (thus marking all copies live) when we encounter a live alias. Two notes: One, this requires a hash table lookup (finding the aliasee summary in the index based on aliasee GUID). However, the impact of this seems to be small compared to the original pre-D56117 thin link time. It could be addressed if we keep the aliasee ValueInfo in the alias summary instead of the aliasee GUID, which I am exploring in a separate patch. Second, we only populate the aliasee GUID field when reading summaries from bitcode (whether we are reading individual summaries and merging on the fly to form the compiled index, or reading in a serialized combined index). Thankfully, that's currently the only way we can get to this code as we don't yet support reading summaries from LLVM assembly directly into a tool that performs the thin link (they must be converted to bitcode first). I added a FIXME, however I have the fix under test already. The easiest fix is to simply populate this field always, which isn't hard, but more likely the change I am exploring to store the ValueInfo instead as described above will subsume this. I don't want to hold up the regression fix for this though. Reviewers: trentxintong Subscribers: mehdi_amini, inglorion, dexonsmith, llvm-commits Differential Revision: https://reviews.llvm.org/D57203 llvm-svn: 352438
2019-01-29 06:27:05 +08:00
continue;
}
Summary->setLive(true);
for (auto Ref : Summary->refs())
[ThinLTO] Fix handling of weak interposable symbols Summary: Keep aliasees alive if their alias is live, otherwise we end up with an alias to a declaration, which is invalid. This can happen when the aliasee is weak and non-prevailing. This fix exposed the fact that we were then attempting to internalize the weak symbol, which was not exported as it was not prevailing. We should not internalize interposable symbols in general, unless this is the prevailing copy, since it can lead to incorrect inlining and other optimizations. Most of the changes in this patch are due to the restructuring required to pass down the prevailing callback. Finally, while implementing the test cases, I found that in the case of a weak aliasee that is still marked not live because its alias isn't live, after dropping the definition we incorrectly marked the declaration with weak linkage when resolving prevailing symbols in the module. This was due to some special case handling for symbols marked WeakLinkage in the summary located before instead of after a subsequent check for the symbol being a declaration. It turns out that we don't actually need this special case handling any more (looking back at the history, when that was added the code was structured quite differently) - we will correctly mark with weak linkage further below when the definition hasn't been dropped. Fixes PR42542. Reviewers: pcc Subscribers: mehdi_amini, inglorion, steven_wu, dexonsmith, dang, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D66264 llvm-svn: 369766
2019-08-23 23:18:58 +08:00
visit(Ref, false);
[ThinLTO] Refine reachability check to fix compile time increase Summary: A recent fix to the ThinLTO whole program dead code elimination (D56117) increased the thin link time on a large MSAN'ed binary by 2x. It's likely that the time increased elsewhere, but was more noticeable here since it was already large and ended up timing out. That change made it so we would repeatedly scan all copies of linkonce symbols for liveness every time they were encountered during the graph traversal. This was needed since we only mark one copy of an aliasee as live when we encounter a live alias. This patch fixes the issue in a more efficient manner by simply proactively visiting the aliasee (thus marking all copies live) when we encounter a live alias. Two notes: One, this requires a hash table lookup (finding the aliasee summary in the index based on aliasee GUID). However, the impact of this seems to be small compared to the original pre-D56117 thin link time. It could be addressed if we keep the aliasee ValueInfo in the alias summary instead of the aliasee GUID, which I am exploring in a separate patch. Second, we only populate the aliasee GUID field when reading summaries from bitcode (whether we are reading individual summaries and merging on the fly to form the compiled index, or reading in a serialized combined index). Thankfully, that's currently the only way we can get to this code as we don't yet support reading summaries from LLVM assembly directly into a tool that performs the thin link (they must be converted to bitcode first). I added a FIXME, however I have the fix under test already. The easiest fix is to simply populate this field always, which isn't hard, but more likely the change I am exploring to store the ValueInfo instead as described above will subsume this. I don't want to hold up the regression fix for this though. Reviewers: trentxintong Subscribers: mehdi_amini, inglorion, dexonsmith, llvm-commits Differential Revision: https://reviews.llvm.org/D57203 llvm-svn: 352438
2019-01-29 06:27:05 +08:00
if (auto *FS = dyn_cast<FunctionSummary>(Summary.get()))
for (auto Call : FS->calls())
[ThinLTO] Fix handling of weak interposable symbols Summary: Keep aliasees alive if their alias is live, otherwise we end up with an alias to a declaration, which is invalid. This can happen when the aliasee is weak and non-prevailing. This fix exposed the fact that we were then attempting to internalize the weak symbol, which was not exported as it was not prevailing. We should not internalize interposable symbols in general, unless this is the prevailing copy, since it can lead to incorrect inlining and other optimizations. Most of the changes in this patch are due to the restructuring required to pass down the prevailing callback. Finally, while implementing the test cases, I found that in the case of a weak aliasee that is still marked not live because its alias isn't live, after dropping the definition we incorrectly marked the declaration with weak linkage when resolving prevailing symbols in the module. This was due to some special case handling for symbols marked WeakLinkage in the summary located before instead of after a subsequent check for the symbol being a declaration. It turns out that we don't actually need this special case handling any more (looking back at the history, when that was added the code was structured quite differently) - we will correctly mark with weak linkage further below when the definition hasn't been dropped. Fixes PR42542. Reviewers: pcc Subscribers: mehdi_amini, inglorion, steven_wu, dexonsmith, dang, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D66264 llvm-svn: 369766
2019-08-23 23:18:58 +08:00
visit(Call.first, false);
}
}
Index.setWithGlobalValueDeadStripping();
unsigned DeadSymbols = Index.size() - LiveSymbols;
LLVM_DEBUG(dbgs() << LiveSymbols << " symbols Live, and " << DeadSymbols
<< " symbols Dead \n");
NumDeadSymbols += DeadSymbols;
NumLiveSymbols += LiveSymbols;
}
// Compute dead symbols and propagate constants in combined index.
void llvm::computeDeadSymbolsWithConstProp(
ModuleSummaryIndex &Index,
const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols,
function_ref<PrevailingType(GlobalValue::GUID)> isPrevailing,
bool ImportEnabled) {
computeDeadSymbols(Index, GUIDPreservedSymbols, isPrevailing);
if (ImportEnabled)
Index.propagateAttributes(GUIDPreservedSymbols);
}
/// Compute the set of summaries needed for a ThinLTO backend compilation of
/// \p ModulePath.
void llvm::gatherImportedSummariesForModule(
StringRef ModulePath,
const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
const FunctionImporter::ImportMapTy &ImportList,
std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex) {
// Include all summaries from the importing module.
ModuleToSummariesForIndex[std::string(ModulePath)] =
ModuleToDefinedGVSummaries.lookup(ModulePath);
// Include summaries for imports.
for (auto &ILI : ImportList) {
auto &SummariesForIndex =
ModuleToSummariesForIndex[std::string(ILI.first())];
const auto &DefinedGVSummaries =
ModuleToDefinedGVSummaries.lookup(ILI.first());
for (auto &GI : ILI.second) {
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
const auto &DS = DefinedGVSummaries.find(GI);
assert(DS != DefinedGVSummaries.end() &&
"Expected a defined summary for imported global value");
Restore "[ThinLTO] Ensure we always select the same function copy to import" This reverts commit r337081, therefore restoring r337050 (and fix in r337059), with test fix for bot failure described after the original description below. In order to always import the same copy of a linkonce function, even when encountering it with different thresholds (a higher one then a lower one), keep track of the summary we decided to import. This ensures that the backend only gets a single definition to import for each GUID, so that it doesn't need to choose one. Move the largest threshold the GUID was considered for import into the current module out of the ImportMap (which is part of a larger map maintained across the whole index), and into a new map just maintained for the current module we are computing imports for. This saves some memory since we no longer have the thresholds maintained across the whole index (and throughout the in-process backends when doing a normal non-distributed ThinLTO build), at the cost of some additional information being maintained for each invocation of ComputeImportForModule (the selected summary pointer for each import). There is an additional map lookup for each callee being considered for importing, however, this was able to subsume a map lookup in the Worklist iteration that invokes computeImportForFunction. We also are able to avoid calling selectCallee if we already failed to import at the same or higher threshold. I compared the run time and peak memory for the SPEC2006 471.omnetpp benchmark (running in-process ThinLTO backends), as well as for a large internal benchmark with a distributed ThinLTO build (so just looking at the thin link time/memory). Across a number of runs with and without this change there was no significant change in the time and memory. (I tried a few other variations of the change but they also didn't improve time or peak memory). The new commit removes a test that no longer makes sense (Transforms/FunctionImport/hotness_based_import2.ll), as exposed by the reverse-iteration bot. The test depends on the order of processing the summary call edges, and actually depended on the old problematic behavior of selecting more than one summary for a given GUID when encountered with different thresholds. There was no guarantee even before that we would eventually pick the linkonce copy with the hottest call edges, it just happened to work with the test and the old code, and there was no guarantee that we would end up importing the selected version of the copy that had the hottest call edges (since the backend would effectively import only one of the selected copies). Reviewers: davidxl Subscribers: mehdi_amini, inglorion, llvm-commits Differential Revision: https://reviews.llvm.org/D48670 llvm-svn: 337184
2018-07-16 23:30:27 +08:00
SummariesForIndex[GI] = DS->second;
}
}
}
/// Emit the files \p ModulePath will import from into \p OutputFilename.
std::error_code llvm::EmitImportsFiles(
StringRef ModulePath, StringRef OutputFilename,
const std::map<std::string, GVSummaryMapTy> &ModuleToSummariesForIndex) {
std::error_code EC;
raw_fd_ostream ImportsOS(OutputFilename, EC, sys::fs::OpenFlags::OF_None);
if (EC)
return EC;
for (auto &ILI : ModuleToSummariesForIndex)
// The ModuleToSummariesForIndex map includes an entry for the current
// Module (needed for writing out the index files). We don't want to
// include it in the imports file, however, so filter it out.
if (ILI.first != ModulePath)
ImportsOS << ILI.first << "\n";
return std::error_code();
}
bool llvm::convertToDeclaration(GlobalValue &GV) {
LLVM_DEBUG(dbgs() << "Converting to a declaration: `" << GV.getName()
<< "\n");
if (Function *F = dyn_cast<Function>(&GV)) {
F->deleteBody();
F->clearMetadata();
F->setComdat(nullptr);
} else if (GlobalVariable *V = dyn_cast<GlobalVariable>(&GV)) {
V->setInitializer(nullptr);
V->setLinkage(GlobalValue::ExternalLinkage);
V->clearMetadata();
V->setComdat(nullptr);
} else {
GlobalValue *NewGV;
if (GV.getValueType()->isFunctionTy())
NewGV =
Function::Create(cast<FunctionType>(GV.getValueType()),
GlobalValue::ExternalLinkage, GV.getAddressSpace(),
"", GV.getParent());
else
NewGV =
new GlobalVariable(*GV.getParent(), GV.getValueType(),
/*isConstant*/ false, GlobalValue::ExternalLinkage,
/*init*/ nullptr, "",
/*insertbefore*/ nullptr, GV.getThreadLocalMode(),
GV.getType()->getAddressSpace());
NewGV->takeName(&GV);
GV.replaceAllUsesWith(NewGV);
return false;
}
return true;
}
/// Fixup prevailing symbol linkages in \p TheModule based on summary analysis.
void llvm::thinLTOResolvePrevailingInModule(
Module &TheModule, const GVSummaryMapTy &DefinedGlobals) {
auto updateLinkage = [&](GlobalValue &GV) {
// See if the global summary analysis computed a new resolved linkage.
const auto &GS = DefinedGlobals.find(GV.getGUID());
if (GS == DefinedGlobals.end())
return;
auto NewLinkage = GS->second->linkage();
if (NewLinkage == GV.getLinkage())
return;
if (GlobalValue::isLocalLinkage(GV.getLinkage()) ||
// Don't internalize anything here, because the code below
// lacks necessary correctness checks. Leave this job to
// LLVM 'internalize' pass.
GlobalValue::isLocalLinkage(NewLinkage) ||
// In case it was dead and already converted to declaration.
GV.isDeclaration())
return;
[ThinLTO] Fix handling of weak interposable symbols Summary: Keep aliasees alive if their alias is live, otherwise we end up with an alias to a declaration, which is invalid. This can happen when the aliasee is weak and non-prevailing. This fix exposed the fact that we were then attempting to internalize the weak symbol, which was not exported as it was not prevailing. We should not internalize interposable symbols in general, unless this is the prevailing copy, since it can lead to incorrect inlining and other optimizations. Most of the changes in this patch are due to the restructuring required to pass down the prevailing callback. Finally, while implementing the test cases, I found that in the case of a weak aliasee that is still marked not live because its alias isn't live, after dropping the definition we incorrectly marked the declaration with weak linkage when resolving prevailing symbols in the module. This was due to some special case handling for symbols marked WeakLinkage in the summary located before instead of after a subsequent check for the symbol being a declaration. It turns out that we don't actually need this special case handling any more (looking back at the history, when that was added the code was structured quite differently) - we will correctly mark with weak linkage further below when the definition hasn't been dropped. Fixes PR42542. Reviewers: pcc Subscribers: mehdi_amini, inglorion, steven_wu, dexonsmith, dang, llvm-commits Tags: #llvm Differential Revision: https://reviews.llvm.org/D66264 llvm-svn: 369766
2019-08-23 23:18:58 +08:00
// Check for a non-prevailing def that has interposable linkage
// (e.g. non-odr weak or linkonce). In that case we can't simply
// convert to available_externally, since it would lose the
// interposable property and possibly get inlined. Simply drop
// the definition in that case.
if (GlobalValue::isAvailableExternallyLinkage(NewLinkage) &&
GlobalValue::isInterposableLinkage(GV.getLinkage())) {
if (!convertToDeclaration(GV))
// FIXME: Change this to collect replaced GVs and later erase
// them from the parent module once thinLTOResolvePrevailingGUID is
// changed to enable this for aliases.
llvm_unreachable("Expected GV to be converted");
} else {
// If all copies of the original symbol had global unnamed addr and
// linkonce_odr linkage, it should be an auto hide symbol. In that case
// the thin link would have marked it as CanAutoHide. Add hidden visibility
// to the symbol to preserve the property.
if (NewLinkage == GlobalValue::WeakODRLinkage &&
GS->second->canAutoHide()) {
assert(GV.hasLinkOnceODRLinkage() && GV.hasGlobalUnnamedAddr());
GV.setVisibility(GlobalValue::HiddenVisibility);
}
LLVM_DEBUG(dbgs() << "ODR fixing up linkage for `" << GV.getName()
<< "` from " << GV.getLinkage() << " to " << NewLinkage
<< "\n");
GV.setLinkage(NewLinkage);
}
// Remove declarations from comdats, including available_externally
// as this is a declaration for the linker, and will be dropped eventually.
// It is illegal for comdats to contain declarations.
auto *GO = dyn_cast_or_null<GlobalObject>(&GV);
if (GO && GO->isDeclarationForLinker() && GO->hasComdat())
GO->setComdat(nullptr);
};
// Process functions and global now
for (auto &GV : TheModule)
updateLinkage(GV);
for (auto &GV : TheModule.globals())
updateLinkage(GV);
for (auto &GV : TheModule.aliases())
updateLinkage(GV);
}
/// Run internalization on \p TheModule based on symmary analysis.
void llvm::thinLTOInternalizeModule(Module &TheModule,
const GVSummaryMapTy &DefinedGlobals) {
// Declare a callback for the internalize pass that will ask for every
// candidate GlobalValue if it can be internalized or not.
auto MustPreserveGV = [&](const GlobalValue &GV) -> bool {
// Lookup the linkage recorded in the summaries during global analysis.
auto GS = DefinedGlobals.find(GV.getGUID());
if (GS == DefinedGlobals.end()) {
// Must have been promoted (possibly conservatively). Find original
// name so that we can access the correct summary and see if it can
// be internalized again.
// FIXME: Eventually we should control promotion instead of promoting
// and internalizing again.
StringRef OrigName =
ModuleSummaryIndex::getOriginalNameBeforePromote(GV.getName());
std::string OrigId = GlobalValue::getGlobalIdentifier(
OrigName, GlobalValue::InternalLinkage,
TheModule.getSourceFileName());
GS = DefinedGlobals.find(GlobalValue::getGUID(OrigId));
if (GS == DefinedGlobals.end()) {
// Also check the original non-promoted non-globalized name. In some
// cases a preempted weak value is linked in as a local copy because
// it is referenced by an alias (IRLinker::linkGlobalValueProto).
// In that case, since it was originally not a local value, it was
// recorded in the index using the original name.
// FIXME: This may not be needed once PR27866 is fixed.
GS = DefinedGlobals.find(GlobalValue::getGUID(OrigName));
assert(GS != DefinedGlobals.end());
}
}
return !GlobalValue::isLocalLinkage(GS->second->linkage());
};
// FIXME: See if we can just internalize directly here via linkage changes
// based on the index, rather than invoking internalizeModule.
internalizeModule(TheModule, MustPreserveGV);
}
/// Make alias a clone of its aliasee.
static Function *replaceAliasWithAliasee(Module *SrcModule, GlobalAlias *GA) {
Function *Fn = cast<Function>(GA->getBaseObject());
ValueToValueMapTy VMap;
Function *NewFn = CloneFunction(Fn, VMap);
// Clone should use the original alias's linkage, visibility and name, and we
// ensure all uses of alias instead use the new clone (casted if necessary).
NewFn->setLinkage(GA->getLinkage());
NewFn->setVisibility(GA->getVisibility());
GA->replaceAllUsesWith(ConstantExpr::getBitCast(NewFn, GA->getType()));
NewFn->takeName(GA);
return NewFn;
}
// Internalize values that we marked with specific attribute
// in processGlobalForThinLTO.
static void internalizeGVsAfterImport(Module &M) {
for (auto &GV : M.globals())
// Skip GVs which have been converted to declarations
// by dropDeadSymbols.
if (!GV.isDeclaration() && GV.hasAttribute("thinlto-internalize")) {
GV.setLinkage(GlobalValue::InternalLinkage);
GV.setVisibility(GlobalValue::DefaultVisibility);
}
}
// Automatically import functions in Module \p DestModule based on the summaries
// index.
Expected<bool> FunctionImporter::importFunctions(
Module &DestModule, const FunctionImporter::ImportMapTy &ImportList) {
LLVM_DEBUG(dbgs() << "Starting import for Module "
<< DestModule.getModuleIdentifier() << "\n");
unsigned ImportedCount = 0, ImportedGVCount = 0;
IRMover Mover(DestModule);
// Do the actual import of functions now, one Module at a time
std::set<StringRef> ModuleNameOrderedList;
for (auto &FunctionsToImportPerModule : ImportList) {
ModuleNameOrderedList.insert(FunctionsToImportPerModule.first());
}
for (auto &Name : ModuleNameOrderedList) {
// Get the module for the import
const auto &FunctionsToImportPerModule = ImportList.find(Name);
assert(FunctionsToImportPerModule != ImportList.end());
Expected<std::unique_ptr<Module>> SrcModuleOrErr = ModuleLoader(Name);
if (!SrcModuleOrErr)
return SrcModuleOrErr.takeError();
std::unique_ptr<Module> SrcModule = std::move(*SrcModuleOrErr);
assert(&DestModule.getContext() == &SrcModule->getContext() &&
"Context mismatch");
// If modules were created with lazy metadata loading, materialize it
// now, before linking it (otherwise this will be a noop).
if (Error Err = SrcModule->materializeMetadata())
return std::move(Err);
auto &ImportGUIDs = FunctionsToImportPerModule->second;
// Find the globals to import
SetVector<GlobalValue *> GlobalsToImport;
for (Function &F : *SrcModule) {
if (!F.hasName())
continue;
auto GUID = F.getGUID();
auto Import = ImportGUIDs.count(GUID);
LLVM_DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing function "
<< GUID << " " << F.getName() << " from "
<< SrcModule->getSourceFileName() << "\n");
if (Import) {
if (Error Err = F.materialize())
return std::move(Err);
if (EnableImportMetadata) {
// Add 'thinlto_src_module' metadata for statistics and debugging.
F.setMetadata(
"thinlto_src_module",
MDNode::get(DestModule.getContext(),
{MDString::get(DestModule.getContext(),
SrcModule->getSourceFileName())}));
}
GlobalsToImport.insert(&F);
}
}
for (GlobalVariable &GV : SrcModule->globals()) {
if (!GV.hasName())
continue;
auto GUID = GV.getGUID();
auto Import = ImportGUIDs.count(GUID);
LLVM_DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing global "
<< GUID << " " << GV.getName() << " from "
<< SrcModule->getSourceFileName() << "\n");
if (Import) {
if (Error Err = GV.materialize())
return std::move(Err);
ImportedGVCount += GlobalsToImport.insert(&GV);
}
}
for (GlobalAlias &GA : SrcModule->aliases()) {
if (!GA.hasName())
continue;
auto GUID = GA.getGUID();
auto Import = ImportGUIDs.count(GUID);
LLVM_DEBUG(dbgs() << (Import ? "Is" : "Not") << " importing alias "
<< GUID << " " << GA.getName() << " from "
<< SrcModule->getSourceFileName() << "\n");
if (Import) {
if (Error Err = GA.materialize())
return std::move(Err);
// Import alias as a copy of its aliasee.
GlobalObject *Base = GA.getBaseObject();
if (Error Err = Base->materialize())
return std::move(Err);
auto *Fn = replaceAliasWithAliasee(SrcModule.get(), &GA);
LLVM_DEBUG(dbgs() << "Is importing aliasee fn " << Base->getGUID()
<< " " << Base->getName() << " from "
<< SrcModule->getSourceFileName() << "\n");
if (EnableImportMetadata) {
// Add 'thinlto_src_module' metadata for statistics and debugging.
Fn->setMetadata(
"thinlto_src_module",
MDNode::get(DestModule.getContext(),
{MDString::get(DestModule.getContext(),
SrcModule->getSourceFileName())}));
}
GlobalsToImport.insert(Fn);
}
}
// Upgrade debug info after we're done materializing all the globals and we
// have loaded all the required metadata!
UpgradeDebugInfo(*SrcModule);
// Link in the specified functions.
if (renameModuleForThinLTO(*SrcModule, Index, &GlobalsToImport))
return true;
if (PrintImports) {
for (const auto *GV : GlobalsToImport)
dbgs() << DestModule.getSourceFileName() << ": Import " << GV->getName()
<< " from " << SrcModule->getSourceFileName() << "\n";
}
if (Mover.move(std::move(SrcModule), GlobalsToImport.getArrayRef(),
[](GlobalValue &, IRMover::ValueAdder) {},
/*IsPerformingImport=*/true))
report_fatal_error("Function Import: link error");
ImportedCount += GlobalsToImport.size();
NumImportedModules++;
}
internalizeGVsAfterImport(DestModule);
NumImportedFunctions += (ImportedCount - ImportedGVCount);
NumImportedGlobalVars += ImportedGVCount;
LLVM_DEBUG(dbgs() << "Imported " << ImportedCount - ImportedGVCount
<< " functions for Module "
<< DestModule.getModuleIdentifier() << "\n");
LLVM_DEBUG(dbgs() << "Imported " << ImportedGVCount
<< " global variables for Module "
<< DestModule.getModuleIdentifier() << "\n");
return ImportedCount;
}
static bool doImportingForModule(Module &M) {
if (SummaryFile.empty())
report_fatal_error("error: -function-import requires -summary-file\n");
Expected<std::unique_ptr<ModuleSummaryIndex>> IndexPtrOrErr =
getModuleSummaryIndexForFile(SummaryFile);
if (!IndexPtrOrErr) {
logAllUnhandledErrors(IndexPtrOrErr.takeError(), errs(),
"Error loading file '" + SummaryFile + "': ");
return false;
}
std::unique_ptr<ModuleSummaryIndex> Index = std::move(*IndexPtrOrErr);
// First step is collecting the import list.
FunctionImporter::ImportMapTy ImportList;
// If requested, simply import all functions in the index. This is used
// when testing distributed backend handling via the opt tool, when
// we have distributed indexes containing exactly the summaries to import.
if (ImportAllIndex)
ComputeCrossModuleImportForModuleFromIndex(M.getModuleIdentifier(), *Index,
ImportList);
else
ComputeCrossModuleImportForModule(M.getModuleIdentifier(), *Index,
ImportList);
// Conservatively mark all internal values as promoted. This interface is
// only used when doing importing via the function importing pass. The pass
// is only enabled when testing importing via the 'opt' tool, which does
// not do the ThinLink that would normally determine what values to promote.
for (auto &I : *Index) {
for (auto &S : I.second.SummaryList) {
if (GlobalValue::isLocalLinkage(S->linkage()))
S->setLinkage(GlobalValue::ExternalLinkage);
}
}
// Next we need to promote to global scope and rename any local values that
// are potentially exported to other modules.
if (renameModuleForThinLTO(M, *Index, nullptr)) {
errs() << "Error renaming module\n";
return false;
}
// Perform the import now.
auto ModuleLoader = [&M](StringRef Identifier) {
return loadFile(std::string(Identifier), M.getContext());
};
FunctionImporter Importer(*Index, ModuleLoader);
Expected<bool> Result = Importer.importFunctions(M, ImportList);
// FIXME: Probably need to propagate Errors through the pass manager.
if (!Result) {
logAllUnhandledErrors(Result.takeError(), errs(),
"Error importing module: ");
return false;
}
return *Result;
}
namespace {
/// Pass that performs cross-module function import provided a summary file.
class FunctionImportLegacyPass : public ModulePass {
public:
/// Pass identification, replacement for typeid
static char ID;
explicit FunctionImportLegacyPass() : ModulePass(ID) {}
/// Specify pass name for debug output
StringRef getPassName() const override { return "Function Importing"; }
bool runOnModule(Module &M) override {
if (skipModule(M))
return false;
return doImportingForModule(M);
}
};
} // end anonymous namespace
PreservedAnalyses FunctionImportPass::run(Module &M,
ModuleAnalysisManager &AM) {
if (!doImportingForModule(M))
return PreservedAnalyses::all();
return PreservedAnalyses::none();
}
char FunctionImportLegacyPass::ID = 0;
INITIALIZE_PASS(FunctionImportLegacyPass, "function-import",
"Summary Based Function Import", false, false)
namespace llvm {
Pass *createFunctionImportPass() {
return new FunctionImportLegacyPass();
}
} // end namespace llvm