[PATCH] llvm-link: Add -override flag to prefer duplicate symbols from one module.
Duncan P. N. Exon Smith
dexonsmith at apple.com
Mon Apr 20 13:59:19 PDT 2015
> On 2015-Apr-20, at 13:28, Luqman Aden <me+llvm at luqman.ca> wrote:
>
> From df1b11c561dc826d8e4836b9e910ac75185bcf05 Mon Sep 17 00:00:00 2001
> From: Luqman Aden <luqman at apple.com>
> Date: Wed, 15 Apr 2015 12:22:32 -0700
> Subject: [PATCH] llvm-link: Add -override flag to prefer one of a duplicate
> symbol.
>
> ---
> include/llvm/Linker/Linker.h | 4 +++-
> lib/Linker/LinkModules.cpp | 20 +++++++++++++++----
> test/tools/llvm-link/Inputs/foo.ll | 4 ++++
> test/tools/llvm-link/hello.ll | 20 +++++++++++++++++++
> test/tools/llvm-link/lit.local.cfg | 1 +
> tools/llvm-link/llvm-link.cpp | 40 ++++++++++++++++++++++++--------------
> 6 files changed, 69 insertions(+), 20 deletions(-)
> create mode 100644 test/tools/llvm-link/Inputs/foo.ll
> create mode 100644 test/tools/llvm-link/hello.ll
> create mode 100644 test/tools/llvm-link/lit.local.cfg
>
> From df1b11c561dc826d8e4836b9e910ac75185bcf05 Mon Sep 17 00:00:00 2001
> From: Luqman Aden <luqman at apple.com>
> Date: Wed, 15 Apr 2015 12:22:32 -0700
> Subject: [PATCH] llvm-link: Add -override flag to prefer one of a duplicate
> symbol.
Luqman and I talked in person, and he added some context for this (seems
to still be missing from llvm-commits?). The idea is to support an LLVM
developer workflow where a subset of the module is extracted to a side
module to be manipulated or optimized differently. Sometime before
CodeGen, these functions are merged back in, and should replace the
copies in the "main" module (regardless of IR linkage types).
This seems like a pretty useful debugging workflow to me, and the
approach in this patch seems about right.
@Rafael, I'd appreciate your opinion on this.
>
> ---
> include/llvm/Linker/Linker.h | 4 +++-
> lib/Linker/LinkModules.cpp | 20 +++++++++++++++----
> test/tools/llvm-link/Inputs/foo.ll | 4 ++++
> test/tools/llvm-link/hello.ll | 20 +++++++++++++++++++
> test/tools/llvm-link/lit.local.cfg | 1 +
> tools/llvm-link/llvm-link.cpp | 40 ++++++++++++++++++++++++--------------
> 6 files changed, 69 insertions(+), 20 deletions(-)
> create mode 100644 test/tools/llvm-link/Inputs/foo.ll
> create mode 100644 test/tools/llvm-link/hello.ll
> create mode 100644 test/tools/llvm-link/lit.local.cfg
>
> diff --git a/include/llvm/Linker/Linker.h b/include/llvm/Linker/Linker.h
> index 5ca815c..c43b90e 100644
> --- a/include/llvm/Linker/Linker.h
> +++ b/include/llvm/Linker/Linker.h
> @@ -68,8 +68,10 @@ public:
> void deleteModule();
>
> /// \brief Link \p Src into the composite. The source is destroyed.
> + /// Passing OverrideSymbols as true will have symbols from Src
> + /// shadow those in the Dest.
> /// Returns true on error.
> - bool linkInModule(Module *Src);
> + bool linkInModule(Module *Src, bool OverrideSymbols = false);
>
> /// \brief Set the composite to the passed-in module.
> void setModule(Module *Dst);
> diff --git a/lib/Linker/LinkModules.cpp b/lib/Linker/LinkModules.cpp
> index 21edc50..3a46a33 100644
> --- a/lib/Linker/LinkModules.cpp
> +++ b/lib/Linker/LinkModules.cpp
> @@ -424,12 +424,17 @@ class ModuleLinker {
>
> DiagnosticHandlerFunction DiagnosticHandler;
>
> + /// For symbol clashes, prefer those from Src.
> + bool OverrideFromSrc;
> +
> public:
> ModuleLinker(Module *dstM, Linker::IdentifiedStructTypeSet &Set, Module *srcM,
> - DiagnosticHandlerFunction DiagnosticHandler)
> + DiagnosticHandlerFunction DiagnosticHandler,
> + bool Override = false)
Why spell this differently from the class member?
Also, are there any callers that don't specify this flag? If not, don't
provide a default.
> : DstM(dstM), SrcM(srcM), TypeMap(Set),
> ValMaterializer(TypeMap, DstM, LazilyLinkGlobalValues),
> - DiagnosticHandler(DiagnosticHandler) {}
> + DiagnosticHandler(DiagnosticHandler),
> + OverrideFromSrc(Override) {}
>
> bool run();
>
> @@ -725,6 +730,12 @@ bool ModuleLinker::getComdatResult(const Comdat *SrcC,
> bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,
> const GlobalValue &Dest,
> const GlobalValue &Src) {
> + // Should we unconditionally use the Src?
> + if (OverrideFromSrc) {
> + LinkFromSrc = true;
> + return false;
> + }
> +
> // We always have to add Src if it has appending linkage.
> if (Src.hasAppendingLinkage()) {
> LinkFromSrc = true;
> @@ -794,6 +805,7 @@ bool ModuleLinker::shouldLinkFromSource(bool &LinkFromSrc,
> assert(!Dest.hasExternalWeakLinkage());
> assert(Dest.hasExternalLinkage() && Src.hasExternalLinkage() &&
> "Unexpected linkage type!");
> +
This seems unrelated to the patch?
> return emitError("Linking globals named '" + Src.getName() +
> "': symbol multiply defined!");
> }
> @@ -1742,9 +1754,9 @@ void Linker::deleteModule() {
> Composite = nullptr;
> }
>
> -bool Linker::linkInModule(Module *Src) {
> +bool Linker::linkInModule(Module *Src, bool OverrideSymbols) {
> ModuleLinker TheLinker(Composite, IdentifiedStructTypes, Src,
> - DiagnosticHandler);
> + DiagnosticHandler, OverrideSymbols);
> bool RetCode = TheLinker.run();
> Composite->dropTriviallyDeadConstantArrays();
> return RetCode;
> diff --git a/test/tools/llvm-link/Inputs/foo.ll b/test/tools/llvm-link/Inputs/foo.ll
> new file mode 100644
> index 0000000..6e06fa5
> --- /dev/null
> +++ b/test/tools/llvm-link/Inputs/foo.ll
> @@ -0,0 +1,4 @@
> +define i32 @foo(i32 %i) {
> +entry:
> + ret i32 4
> +}
We usually put lib/Linker tests in "test/Linker".
> diff --git a/test/tools/llvm-link/hello.ll b/test/tools/llvm-link/hello.ll
> new file mode 100644
> index 0000000..c124864
> --- /dev/null
> +++ b/test/tools/llvm-link/hello.ll
> @@ -0,0 +1,20 @@
> +; RUN: llvm-as %s -o %t-hello.bc
> +; RUN: llvm-as %p/Inputs/foo.ll -o %t-foo.bc
Why not just use llvm-link directly?
Also, I recommend this naming scheme:
test/Linker/manual-override.ll
test/Linker/Inputs/manual-override.ll
(Or whatever you want to name the tests, but I wouldn't call it "hello".)
I'd also use `%S` instead of `%p`, since last I checked only the
former is actually documented at llvm.org.
Also, it might be worth testing both command-line orderings.
> +; RUN: llvm-link %t-hello.bc -override %t-foo.bc -S | FileCheck %s
> +
> +
> +; CHECK-LABEL: define i32 @foo
> +; CHECK-NEXT: entry:
> +; CHECK-NEXT: ret i32 4
> +define i32 @foo(i32 %i) {
> +entry:
> + %add = add nsw i32 %i, %i
> + ret i32 %add
> +}
> +
> +; Function Attrs: nounwind ssp uwtable
> +define i32 @main(i32 %argc, i8** %argv) {
> +entry:
> + %a = call i32 @foo(i32 2)
> + ret i32 %a
> +}
> diff --git a/test/tools/llvm-link/lit.local.cfg b/test/tools/llvm-link/lit.local.cfg
> new file mode 100644
> index 0000000..c6106e4
> --- /dev/null
> +++ b/test/tools/llvm-link/lit.local.cfg
> @@ -0,0 +1 @@
> +config.suffixes = ['.ll']
Since the tests should probably be moved to `test/Linker`, we probably
don't need this.
> diff --git a/tools/llvm-link/llvm-link.cpp b/tools/llvm-link/llvm-link.cpp
> index 6924aa5..54d49f5 100644
> --- a/tools/llvm-link/llvm-link.cpp
> +++ b/tools/llvm-link/llvm-link.cpp
> @@ -38,6 +38,11 @@ static cl::list<std::string>
> InputFilenames(cl::Positional, cl::OneOrMore,
> cl::desc("<input bitcode files>"));
>
> +static cl::list<std::string>
> +OverridingInputs("override", cl::ZeroOrMore, cl::value_desc("filename"),
> + cl::desc("input bitcode file which can "
> + "override previously defined symbol"));
> +
> static cl::opt<std::string>
> OutputFilename("o", cl::desc("Override output filename"), cl::init("-"),
> cl::value_desc("filename"));
> @@ -109,24 +114,29 @@ int main(int argc, char **argv) {
> auto Composite = make_unique<Module>("llvm-link", Context);
> Linker L(Composite.get(), diagnosticHandler);
>
> - for (unsigned i = 0; i < InputFilenames.size(); ++i) {
> - std::unique_ptr<Module> M = loadFile(argv[0], InputFilenames[i], Context);
> - if (!M.get()) {
> - errs() << argv[0] << ": error loading file '" <<InputFilenames[i]<< "'\n";
> - return 1;
> - }
> + auto linkFiles = [&](const cl::list<std::string>& Files, bool Override) {
> + for (auto File : Files) {
> + std::unique_ptr<Module> M = loadFile(argv[0], File, Context);
> + if (!M.get()) {
> + errs() << argv[0] << ": error loading file '" << File << "'\n";
> + exit(1);
> + }
>
> - if (verifyModule(*M, &errs())) {
> - errs() << argv[0] << ": " << InputFilenames[i]
> - << ": error: input module is broken!\n";
> - return 1;
> - }
> + if (verifyModule(*M, &errs())) {
> + errs() << argv[0] << ": " << File
> + << ": error: input module is broken!\n";
> + exit(1);
> + }
>
> - if (Verbose) errs() << "Linking in '" << InputFilenames[i] << "'\n";
> + if (Verbose) errs() << "Linking in '" << File << "'\n";
>
> - if (L.linkInModule(M.get()))
> - return 1;
> - }
> + if (L.linkInModule(M.get(), Override))
> + exit(1);
> + }
> + };
> +
> + linkFiles(InputFilenames, false);
It'd be nice for the refactoring part of this (i.e., the part that
changes all the indentation) to be in a separate NFC (no functionality
change) commit so it's easier to see what has really changed.
> + linkFiles(OverridingInputs, true);
>
> if (DumpAsm) errs() << "Here's the assembly:\n" << *Composite;
>
> --
> 2.3.0 (Apple Git-54)
>
>
More information about the llvm-commits
mailing list