[PATCH] D21943: Add 'thin_lto_imported' metadata to imported function

[Teresa Johnson via llvm-commits]

tejohnson added inline comments.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:597
@@ +596,3 @@
+                      llvm::MDNode::get(DestModule.getContext(),
+                                        llvm::ArrayRef<llvm::Metadata *>()));
+        GlobalsToImport.insert(&F);
----------------
Prazek wrote:
> tejohnson wrote:
> > eraman wrote:
> > > It will be more useful to name this something like thin_lto_src_module and attach the source module's name.  
> > I like the module name idea as well. We'd want to ensure that the module name MDString is uniqued though.
> 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.
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.

================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:614
@@ -609,3 +613,3 @@
     }
-    for (auto &GV : SrcModule->aliases()) {
-      if (!GV.hasName())
+    for (GlobalAlias &GA : SrcModule->aliases()) {
+      if (!GA.hasName())
----------------
Prazek wrote:
> tejohnson wrote:
> > 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?
> 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.
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.


Repository:
  rL LLVM

http://reviews.llvm.org/D21943