This is an archive of the discontinued LLVM Phabricator instance.

LTO: Port the new LTO API to ModuleSymbolTable.
ClosedPublic

Authored by pcc on Nov 23 2016, 4:10 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 79169.Nov 23 2016, 4:10 PM
pcc retitled this revision from to LTO: Port the new LTO API to ModuleSymbolTable..
pcc updated this object.
pcc added a reviewer: mehdi_amini.
pcc added subscribers: llvm-commits, rafael.
mehdi_amini added inline comments.Nov 23 2016, 9:07 PM
llvm/include/llvm/LTO/LTO.h
104 ↗(On Diff #79169)

Why don't you take all the members here?
It seems strange to see the initialization and then File->Mod = ...

mehdi_amini added inline comments.Nov 23 2016, 9:18 PM
llvm/include/llvm/LTO/LTO.h
166 ↗(On Diff #79169)

Can you turn SymTab and File to be reference?

171 ↗(On Diff #79169)

Non relevant for this patch, but it'd be nice to document getName vs getIRName, I always forget when I look at this.

248 ↗(On Diff #79169)

Looks like something to add to ModuleSymbolTable.
(I don't mean it as a blocker for this patch)

pcc updated this revision to Diff 79858.Nov 30 2016, 7:56 PM
pcc marked 2 inline comments as done.
  • Address review comments
llvm/include/llvm/LTO/LTO.h
104 ↗(On Diff #79169)

We need to create the module using the LLVMContext owned by this class.

I suppose that given that we need to do that we might as well be consistent with the other members and let MBRef be default initialized as well.

166 ↗(On Diff #79169)

Done for SymTab. File may be null (see addRegularLTO) so I've left it as a pointer.

171 ↗(On Diff #79169)

It looks like getIRName is only used by the implementation, so I've removed it in r288302 and added documentation for getName.

248 ↗(On Diff #79169)

Maybe not, given our discussion on D27073.

mehdi_amini added inline comments.Nov 30 2016, 8:39 PM
llvm/include/llvm/LTO/LTO.h
104 ↗(On Diff #79169)

OK, I missed this! But indeed I feel it is less confusing now.

171 ↗(On Diff #79169)

It seems you need to rebase this patch?

pcc updated this revision to Diff 79875.Nov 30 2016, 11:19 PM
pcc marked an inline comment as done.
  • Refresh
mehdi_amini added inline comments.Dec 9 2016, 6:46 PM
llvm/include/llvm/LTO/LTO.h
84 ↗(On Diff #79875)

It seems there is not much that still requires us to keep Mod here, what is fundamentally the remaining issue?

pcc added inline comments.Dec 9 2016, 7:43 PM
llvm/include/llvm/LTO/LTO.h
84 ↗(On Diff #79875)

The major ones are:

  • We need something to own the module
  • We need to be able to query the source file name
  • We need it to be able to extract used bits in add*LTO

Essentially we need it to access information that will later be stored in the bitcode symbol table. But until that's ready there's no harm in accessing the module directly.

mehdi_amini added inline comments.Dec 9 2016, 8:39 PM
llvm/include/llvm/LTO/LTO.h
84 ↗(On Diff #79875)

Owning the module is only useful for the two other parts right? Technically we could store the source file name directly and extract the "used" early.
The latter seems overkill to do now considering we're moving to a bitcode symbol table.

mehdi_amini accepted this revision.Dec 9 2016, 8:47 PM
mehdi_amini edited edge metadata.

LGTM

This revision is now accepted and ready to land.Dec 9 2016, 8:47 PM
pcc added inline comments.Dec 13 2016, 11:44 AM
llvm/include/llvm/LTO/LTO.h
84 ↗(On Diff #79875)

An owner is also needed for the GlobalValues stored in ModuleSymbolTable.

But that's just another example of something that can be refactored with the bitcode symbol table.

This revision was automatically updated to reflect the committed changes.