This is an archive of the discontinued LLVM Phabricator instance.

Initial implementation of link-time optimization for the new COFF linker.
ClosedPublic

Authored by pcc on May 28 2015, 5:30 PM.

Diff Detail

Event Timeline

pcc updated this revision to Diff 26761.May 28 2015, 5:30 PM
pcc retitled this revision from to Initial implementation of link-time optimization for the new COFF linker..
pcc updated this object.
pcc edited the test plan for this revision. (Show Details)
pcc added a reviewer: ruiu.
pcc added subscribers: rafael, Unknown Object (MLST).
ruiu edited edge metadata.May 28 2015, 8:52 PM

This is very small amount code, and all the things seem to fit naturally to the existing things. Very nice.

COFF/InputFiles.cpp
251

Use parentheses for ifs having elses for consistency.

261

Ditto

COFF/InputFiles.h
162

Can you add a brief comment for this class saying that this is for LTO, just like I did for other classes?

168

Use override instead of virtual.

171

Add using llvm::LTOModule at beginning of this file.

176

Name -> Filename.

COFF/SymbolTable.cpp
63

Move this piece of code after addArchive(ArchiveFile *) for consistent order.

235

Remove this blank line.

239

Use real type instead of auto.

252

You want to cache size() -- for (unsigned I = 1, E = BitcodeFiles.size(), I != E ...)

260–278

What does this code do? Why do we need to insert new Undefined symbols at this point?

Ah, I think I understand what you are doing. LTO object file is added after all files are resolved, and at this moment we are about to add "duplicate" symbols, correct?

Maybe a better approach is to tweak Defined::compare so that any symbol whose type is of DefinedBitcode always "win" over any other symbols.

280

You want to return addFile's return value.

COFF/SymbolTable.h
62

Also mention that that this function is called after all files are added and before writer writes results to a file.

COFF/Symbols.cpp
85

Use new instead of make_unique with consistency with other lines in this function.

ruiu added inline comments.May 29 2015, 8:37 AM
COFF/SymbolTable.cpp
278

On second thought, I'm now thinking that replacing DefinedBitcode symbols with regular defined symbols can be done without going through addFile. We have a list of symbols which need to be replaced, and we also have symbols in this file that should replace them. So we can just do that by overriding Symbol's Body pointers.

Not sure which is better, so it's just an idea.

pcc added inline comments.May 29 2015, 10:47 AM
COFF/SymbolTable.cpp
278

Ah, I think I understand what you are doing. LTO object file is added after all files are resolved, and at this moment we are about to add "duplicate" symbols, correct?

Correct.

Maybe a better approach is to tweak Defined::compare so that any symbol whose type is of DefinedBitcode always "win" over any other symbols.

Do you mean that non-DefinedBitcode symbols should win over DefinedBitcode symbols? I did consider that approach to start with, but the problem with that approach is that it would prevent us from diagnosing multiply defined symbols correctly. Consider the case where a.o (bitcode) and b.o (COFF) both define a strong symbol 'foo'. When linking a.o with b.o, when we add b.o we will need to diagnose a conflict between a DefinedBitcode and a DefinedRegular. When linking a.o on its own, we have exactly the same situation when we add the LTO object, but we shouldn't diagnose.

On second thought, I'm now thinking that replacing DefinedBitcode symbols with regular defined symbols can be done without going through addFile. We have a list of symbols which need to be replaced, and we also have symbols in this file that should replace them. So we can just do that by overriding Symbol's Body pointers.

I prefer this approach. I'm a little concerned about duplication of logic between here and SymbolTable::resolve (especially once we add more COFF features), but maybe that should be dealt with later if it becomes a problem.

pcc updated this revision to Diff 26816.May 29 2015, 1:42 PM
pcc edited edge metadata.
  • Address most review comments
  • Use custom symbol resolution during LTO
COFF/InputFiles.cpp
251

Done

261

Done

COFF/InputFiles.h
162

Done

168

Done

171

Done

176

Done

COFF/SymbolTable.cpp
63

Done

235

Done

239

Done

252

Done

278

Done

280

This code is now gone.

COFF/SymbolTable.h
62

Done

COFF/Symbols.cpp
85

Done

ruiu accepted this revision.May 29 2015, 1:51 PM
ruiu edited edge metadata.

LGTM

COFF/Driver.cpp
126

Add a comment saying that initialize for LTO.

COFF/SymbolTable.cpp
263

Looks like Obj leaks here.

271

This can be Symbol * instead of Symbol *&.

274

Can't this fit in 80 columns? (If not, never mind).

This revision is now accepted and ready to land.May 29 2015, 1:51 PM
pcc updated this revision to Diff 26825.May 29 2015, 3:02 PM
pcc edited edge metadata.
  • Address review comments
pcc added a comment.May 29 2015, 3:02 PM

(waiting for D10114)

COFF/Driver.cpp
126

Done

COFF/SymbolTable.cpp
263

Fixed.

271

Done

pcc updated this revision to Diff 26911.Jun 1 2015, 10:58 AM

Rebase

This revision was automatically updated to reflect the committed changes.