From 239c831de4d09530ebd4e254f9248764792afa5e Mon Sep 17 00:00:00 2001 From: Bill Wendling Date: Mon, 18 Jul 2022 12:05:53 -0700 Subject: [PATCH] Add switch to use "source_filename" instead of a hash ID for globally promoted local During LTO a local promoted to a global gets a unique suffix based on a hash of the module IR. This means that changes in the local's module can affect the contents in another module that imported it (because the name of the imported promoted local is changed, but that doesn't reflect a real change in the importing module). So any tool that's validating changes to the importing module will see a superficial change. Instead of using the module hash, we can use the "source_filename" if it exists to generate a unique identifier that doesn't change due to LTO shenanigans. Differential Revision: https://reviews.llvm.org/D128863 --- llvm/include/llvm/IR/ModuleSummaryIndex.h | 9 ++++-- llvm/lib/Transforms/Utils/FunctionImportUtils.cpp | 20 +++++++++++++ .../ThinLTO/X86/Inputs/promote-local-name-1.ll | 20 +++++++++++++ llvm/test/ThinLTO/X86/promote-local-name.ll | 33 ++++++++++++++++++++++ 4 files changed, 80 insertions(+), 2 deletions(-) create mode 100644 llvm/test/ThinLTO/X86/Inputs/promote-local-name-1.ll create mode 100644 llvm/test/ThinLTO/X86/promote-local-name.ll diff --git a/llvm/include/llvm/IR/ModuleSummaryIndex.h b/llvm/include/llvm/IR/ModuleSummaryIndex.h index 468773a..203f7b3 100644 --- a/llvm/include/llvm/IR/ModuleSummaryIndex.h +++ b/llvm/include/llvm/IR/ModuleSummaryIndex.h @@ -1465,10 +1465,15 @@ public: /// Convenience method for creating a promoted global name /// for the given value name of a local, and its original module's ID. static std::string getGlobalNameForLocal(StringRef Name, ModuleHash ModHash) { + std::string Suffix = utostr((uint64_t(ModHash[0]) << 32) | + ModHash[1]); // Take the first 64 bits + return getGlobalNameForLocal(Name, Suffix); + } + + static std::string getGlobalNameForLocal(StringRef Name, StringRef Suffix) { SmallString<256> NewName(Name); NewName += ".llvm."; - NewName += utostr((uint64_t(ModHash[0]) << 32) | - ModHash[1]); // Take the first 64 bits + NewName += Suffix; return std::string(NewName.str()); } diff --git a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp index 8e6d462..a30f920 100644 --- a/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp +++ b/llvm/lib/Transforms/Utils/FunctionImportUtils.cpp @@ -12,8 +12,18 @@ //===----------------------------------------------------------------------===// #include "llvm/Transforms/Utils/FunctionImportUtils.h" +#include "llvm/Support/CommandLine.h" using namespace llvm; +/// Uses the "source_filename" instead of a Module hash ID for the suffix of +/// promoted locals during LTO. NOTE: This requires that the source filename +/// has a unique name / path to avoid name collisions. +static cl::opt UseSourceFilenameForPromotedLocals( + "use-source-filename-for-promoted-locals", cl::Hidden, + cl::desc("Uses the source file name instead of the Module hash. " + "This requires that the source filename has a unique name / " + "path to avoid name collisions.")); + /// Checks if we should import SGV as a definition, otherwise import as a /// declaration. bool FunctionImportGlobalProcessing::doImportAsDefinition( @@ -94,9 +104,19 @@ bool FunctionImportGlobalProcessing::isNonRenamableLocal( std::string FunctionImportGlobalProcessing::getPromotedName(const GlobalValue *SGV) { assert(SGV->hasLocalLinkage()); + // For locals that must be promoted to global scope, ensure that // the promoted name uniquely identifies the copy in the original module, // using the ID assigned during combined index creation. + if (UseSourceFilenameForPromotedLocals && + !SGV->getParent()->getSourceFileName().empty()) { + SmallString<256> Suffix(SGV->getParent()->getSourceFileName()); + std::replace_if(std::begin(Suffix), std::end(Suffix), + [&](char ch) { return !isAlnum(ch); }, '_'); + return ModuleSummaryIndex::getGlobalNameForLocal( + SGV->getName(), Suffix); + } + return ModuleSummaryIndex::getGlobalNameForLocal( SGV->getName(), ImportIndex.getModuleHash(SGV->getParent()->getModuleIdentifier())); diff --git a/llvm/test/ThinLTO/X86/Inputs/promote-local-name-1.ll b/llvm/test/ThinLTO/X86/Inputs/promote-local-name-1.ll new file mode 100644 index 0000000..5e161fb --- /dev/null +++ b/llvm/test/ThinLTO/X86/Inputs/promote-local-name-1.ll @@ -0,0 +1,20 @@ +source_filename = "llvm/test/LTO/X86/promote-local-name-1.ll" + +target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-unknown-linux-gnu" + +@baz = internal constant i32 10, align 4 + +; Function Attrs: noinline nounwind uwtable +define i32 @b() { +entry: + %call = call i32 @foo() + ret i32 %call +} + +; Function Attrs: noinline nounwind uwtable +define internal i32 @foo() { +entry: + %0 = load i32, i32* @baz, align 4 + ret i32 %0 +} diff --git a/llvm/test/ThinLTO/X86/promote-local-name.ll b/llvm/test/ThinLTO/X86/promote-local-name.ll new file mode 100644 index 0000000..65c6298 --- /dev/null +++ b/llvm/test/ThinLTO/X86/promote-local-name.ll @@ -0,0 +1,33 @@ +; Test the "-use-source-filename-for-promoted-locals" flag. + +; Do setup work for all below tests: generate bitcode and combined index +; RUN: opt -module-summary -module-hash %s -o %t.bc +; RUN: opt -module-summary -module-hash %p/Inputs/promote-local-name-1.ll -o %t1.bc +; RUN: llvm-lto -thinlto-action=thinlink -o %t2.bc %t.bc %t1.bc + +; This module will import b() which should cause the copy of foo and baz from +; that module (%t1.bc) to be imported. Check that the imported reference's +; promoted name matches the imported copy. We check for a specific name, +; because the result of the "-use-source-filename-for-promoted-locals" flag +; should be deterministic. + +; RUN: llvm-lto -use-source-filename-for-promoted-locals -thinlto-action=import %t.bc -thinlto-index=%t2.bc -o - | llvm-dis -o - | FileCheck %s + +; CHECK: @baz.llvm.llvm_test_LTO_X86_promote_local_name_1_ll = internal constant i32 10, align 4 +; CHECK: call i32 @foo.llvm.llvm_test_LTO_X86_promote_local_name_1_ll +; CHECK: define available_externally hidden i32 @foo.llvm.llvm_test_LTO_X86_promote_local_name_1_ll() + +source_filename = "llvm/test/ThinLTO/X86/promote-local-name.ll" +target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128" +target triple = "x86_64-unknown-linux-gnu" + +; Function Attrs: noinline nounwind uwtable +define i32 @main() { +entry: + %retval = alloca i32, align 4 + store i32 0, i32* %retval, align 4 + %call = call i32 (...) @b() + ret i32 %call +} + +declare i32 @b(...) -- 2.7.4