This is an archive of the discontinued LLVM Phabricator instance.

[LTO] Switch to the new resolution-based API.
ClosedPublic

Authored by davide on Sep 12 2016, 8:05 PM.

Details

Summary

This should be all the logic. Before reviewing the style/code appeareance, I'd like to ensure that this is the right direction, thanks.

Left to be done:

  • There are two tests failing, the one which include inline asm (currently investigating).
  • The old LTO code wasn't removed yet. I found that it makes the patch less readable, and that can be removed separately.

Diff Detail

Event Timeline

davide updated this revision to Diff 71102.Sep 12 2016, 8:05 PM
davide retitled this revision from to [LTO] Switch to the new resolution-based API..
davide updated this object.
davide added reviewers: rafael, pcc, Bigcheese, tejohnson.
davide added a subscriber: llvm-commits.
ruiu added a subscriber: ruiu.Sep 12 2016, 8:15 PM

I want to see the old code being removed in this patch to see deltas.

In D24492#540718, @ruiu wrote:

I want to see the old code being removed in this patch to see deltas.

Fair enough, I'll do that tomorrow.

Back to more interesting business, the two asm inline tests that are failing are also failing with the gold plugin, so that might inidcate a bug in lib/LTO. Here's a synthetic testcase.

$ /usr/bin/ld.gold -shared -m elf_x86_64 -o /home/davide/work/llvm/build/test/tools/gold/X86/Output/module_asm.ll.tmp2 -plugin /home/davide/work/llvm/build/./lib/LLVMgold.so /home/davide/work/llvm/build/test/tools/gold/X86/Output/module_asm.ll.tmp.o
ld.gold: ../lib/Linker/IRMover.cpp:1240: llvm::Error (anonymous namespace)::IRLinker::run(): Assertion `!GV->isDeclaration()' failed.
; RUN: llvm-as %s -o %t.o
; RUN: %gold -shared -m elf_x86_64 -o %t2 -plugin %llvmshlibdir/LLVMgold.so %t.o

target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

module asm ".weak patatino"
module asm ".equ patatino, foo"

declare void @patatino()

define void @foo() {
  ret void
}

define void @_start() {
  call void @patatino()
  ret void
}

I opened a report for the gold-plugin bug (https://llvm.org/bugs/show_bug.cgi?id=30363). I'll take a closer look tomorrow.

ruiu added a comment.Sep 13 2016, 1:05 PM

Davide,

Sure. Go ahead with your patch first.

davide updated this revision to Diff 71405.Sep 14 2016, 11:36 AM

Removed dead code as per Rui's request.
Also, for whoever cares, here's the output of diffstat:

33 files changed, 209 insertions(+), 432 deletions(-)

ruiu added a comment.Sep 14 2016, 11:48 AM

This patch contains complex code with almost no comment. Please explain in the code about what you are doing.

ELF/SymbolTable.cpp
116–120 ↗(On Diff #71405)

Why don't you keep the old interface and replace only the content of LTO.cpp? I think that was a good separation.

pcc added inline comments.Sep 14 2016, 12:05 PM
ELF/SymbolTable.cpp
213 ↗(On Diff #71405)

You should replace all uses of IRObjectFile elsewhere with lto::InputFile. Then you won't need to create a new InputFile here, you can just reuse the one in the BitcodeFile.

225 ↗(On Diff #71405)

You shouldn't need to do this, see r281437.

234 ↗(On Diff #71405)

This isn't right, this should test whether the SymbolBody is a DefinedBitcode and is defined by this BitcodeFile.

252 ↗(On Diff #71405)

There's no need to save objects to files, you can just use raw_string_ostream and operate on the object file directly in memory.

davide updated this revision to Diff 72753.Sep 27 2016, 6:56 PM

Addressed comments. I'm working to remove the call to getComdatSymbolTable(). Other than that it should be ready for another look.

rafael added inline comments.Sep 28 2016, 9:49 AM
lld/ELF/InputFiles.cpp
627–628

You can use GlobalValue::VisibilityTypes to make the argument type clear.

640–645

static

642

You don't need the llvm:: prefixes.
Do you really want a StringSaver by value?

653

Simplify to just

uint8_t Type = ObjSym.isTLS() ? STT_TLS : STT_NOTYPE;

654

Simplify to

uint8_t Visibility = mapVisibility(ObjSym.getVisibility());

659

You have lost HasUnnamedAddr. Where do you compensate for that?

davide updated this revision to Diff 72851.Sep 28 2016, 10:18 AM

Rafael's comments.

lld/ELF/InputFiles.cpp
659

I think it's not needed anymore becuase lib/LTO will compute it based on GlobalResolution.
I'm garbage collecting it for now.

ruiu added inline comments.Sep 28 2016, 10:29 AM
lld/ELF/Error.h
36

You are not using Msg.

lld/ELF/LTO.cpp
149

Filenames is not a large vector, so please use std::string for simplicity.

154–155

I'd make getOutputFilename a pure function that returns std::string so that we can do

Filenames[Task] = getOutputFilename(Filename, MaxTasks > 1 ? Task : -1);
davide updated this revision to Diff 72860.Sep 28 2016, 10:37 AM

Rui's comments.

pcc added inline comments.Sep 28 2016, 10:43 AM
lld/ELF/LTO.cpp
149

Once you make the change below, Filenames will be unused and you can remove it.

163

This could be if (!Buff[I].empty()), then you can make this a for-range loop.

pcc added inline comments.Sep 28 2016, 10:44 AM
lld/ELF/LTO.cpp
151

Rename to AddStream.

rafael added inline comments.Sep 28 2016, 10:56 AM
lld/ELF/InputFiles.cpp
672

You don't need the llvm prefix.

lld/ELF/LTO.cpp
67

Just inline the variable into the single use.

88–91

You don't need the ()

122

Add a comment on why you need the undefined check.

ruiu added inline comments.Sep 28 2016, 11:02 AM
lld/ELF/Error.h
38

Do you need c_str()?

pcc added inline comments.Sep 28 2016, 11:06 AM
lld/ELF/LTO.cpp
79

You could just assign without the if statement here and below.

davide updated this revision to Diff 72867.Sep 28 2016, 11:33 AM
davide added inline comments.
lld/ELF/LTO.cpp
163

hmm, I still need I to be passed to saveBuffer, no? Do you want me to use a separate variable incremented at each iteration?

pcc added inline comments.Sep 28 2016, 11:47 AM
lld/ELF/LTO.cpp
163

No, sorry, what you have is fine.

164

I would remove this comment as it doesn't help readers understand the code.

davide updated this revision to Diff 72870.Sep 28 2016, 11:55 AM

Rebased on top of

commit de07c6a10ee2cbf5877e74fdbbe1d8d3f1d343a4
Author: Etienne Bergeron <etienneb@google.com>
Date:   Wed Sep 28 18:04:07 2016 +0000

+ removed comment as suggested by Peter.

ruiu added inline comments.Sep 28 2016, 12:12 PM
lld/ELF/InputFiles.cpp
648–650

Remove ().

lld/ELF/LTO.cpp
88–92

Remove llvm::.

I don't think your code is prepared to see LtoObj being null. Maybe you want to return a new instance regardless of HasError value?

104–106

Why do you want to return here immediately if there was an error before this function?

davide updated this revision to Diff 72883.Sep 28 2016, 1:02 PM
davide added inline comments.
lld/ELF/LTO.cpp
104–106

Ooops, fixed, thanks.

ruiu added inline comments.Sep 28 2016, 1:09 PM
lld/ELF/LTO.cpp
131–132

Stray }?

136

auto -> BasicSymbolRef &

163
if (Buff[I].empty())
  continue;
davide updated this revision to Diff 72886.Sep 28 2016, 1:23 PM
davide added inline comments.
lld/ELF/LTO.cpp
131–138

hmm, no, just clang-formatted incorrectly. Weird as I have a pre-hook for that that didn't kick in.

ruiu added inline comments.Sep 28 2016, 1:43 PM
lld/ELF/LTO.cpp
131–138

It seems that it is still formatted weirdly.

davide updated this revision to Diff 72890.Sep 28 2016, 1:47 PM

run clang-format manually and it seems fine on my side.

pcc accepted this revision.Sep 28 2016, 4:40 PM
pcc edited edge metadata.

LGTM with nit. Thanks for working on this!

lld/ELF/InputFiles.cpp
641

Unused argument

This revision is now accepted and ready to land.Sep 28 2016, 4:40 PM
ruiu accepted this revision.Sep 28 2016, 5:19 PM
ruiu added a reviewer: ruiu.

LGTM

This revision was automatically updated to reflect the committed changes.