Page MenuHomePhabricator

Add 'thin_lto_imported' metadata to imported function
ClosedPublic

Authored by Prazek on Jul 1 2016, 3:48 PM.

Diff Detail

Event Timeline

Prazek updated this revision to Diff 62557.Jul 1 2016, 3:48 PM
Prazek retitled this revision from to Add 'thin_lto_imported' metadata to imported function.
Prazek updated this object.
Prazek added reviewers: tejohnson, eraman.
Prazek added a subscriber: llvm-commits.
Prazek edited reviewers, added: mehdi_amini; removed: eraman.Jul 1 2016, 3:52 PM
Prazek set the repository for this revision to rL LLVM.
Prazek added a subscriber: eraman.
eraman added inline comments.Jul 1 2016, 4:18 PM
lib/Transforms/IPO/FunctionImport.cpp
597

It will be more useful to name this something like thin_lto_src_module and attach the source module's name.

tejohnson added inline comments.Jul 1 2016, 4:39 PM
lib/Transforms/IPO/FunctionImport.cpp
597

I like the module name idea as well. We'd want to ensure that the module name MDString is uniqued though.

614

Is the GV->GA name change in this block needed? Ah I see from the patch description that this was done for readability - can you make the renaming here and in the above loop a separate earlier NFC patch?

Prazek added inline comments.Jul 1 2016, 8:32 PM
lib/Transforms/IPO/FunctionImport.cpp
597

tl;dr I don't think there is problem with that.

do we really need to ensure that? In the worst case we will merge 2 the same nodes into one and we will not be able to distinguish one from another, but is there any way to make a unique name, that is readable to human?

There are also 'distinct' nodes, but I am not sure if we should use that.

614

What is NFC? Internet tells me "no fat chicks", but then my context analysis fails.
But I guess you mean to just make it separate patch. Can I get lgtm on this rename, so I won't have to post it in phabricator.

tejohnson added inline comments.Jul 1 2016, 9:29 PM
lib/Transforms/IPO/FunctionImport.cpp
597

I just meant that we don't want to have N copies of a full module name if we have N imports from that module. So hopefully it could be done in a way that each unique module name was an MDString and its metadata id would be referenced by the thin_lto_src_module metadata on the N imported functions.

614

It means No Functional Change. Since it is a simple renaming that would apply and you don't need to submit a patch for review in phab. See other commits on the llvm-commits mailing list with "(NFC)" on the subject line for reference.

Prazek added inline comments.Jul 2 2016, 8:53 AM
lib/Transforms/IPO/FunctionImport.cpp
597

oh yes, of course.

Prazek updated this revision to Diff 62948.Jul 6 2016, 1:09 PM
Prazek updated this object.
Prazek removed rL LLVM as the repository for this revision.
Prazek marked 8 inline comments as done.
tejohnson accepted this revision.Jul 6 2016, 1:18 PM
tejohnson edited edge metadata.

Prefer "thinlto_src_module" (s/thin_lto/thinlto) for consistency with other ThinLTO references in llvm.

LGTM with that change.

This revision is now accepted and ready to land.Jul 6 2016, 1:18 PM
This revision was automatically updated to reflect the committed changes.