[PATCH] D43077: [ThinLTO] Import external globals

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Nice! Glad to hear there are some measurable improvements. A few comments/suggestions below.



================
Comment at: lib/Linker/IRMover.cpp:1442
+      IRMover::StructTypeKeyInfo::KeyTy((*NI).second) == Key)
+    return (*NI).second;
+  return *I;
----------------
I'm not as familiar with the struct mapping - what happens without this change? Why did importing variables necessitate it?


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:245
+    StringMap<FunctionImporter::ExportSetTy> *ExportLists) {
+  if (Summary.modulePath().empty())
+    return;
----------------
Why would the modulePath be empty? I think we should only have summaries during the thin link for defs in IR. Oh - answering my own question, I think - this is for regular LTO module summaries which are added to a dummy module. Please add comment.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:257
+    for (auto &RefSummary : VI.getSummaryList())
+      if (RefSummary->getSummaryKind() == GlobalValueSummary::GlobalVarKind &&
+          !RefSummary->modulePath().empty() &&
----------------
Add a comment about why we are limiting to only variable refs with external linkage and no other refs.


================
Comment at: test/Transforms/FunctionImport/funcimport.ll:109
 ; INSTLIMDEF-DAG: Import globalfunc2
-; INSTLIMDEF-DAG: 13 function-import - Number of functions imported
+; INSTLIMDEF-DAG: 14 function-import - Number of functions imported
 ; CHECK-DAG: !0 = !{!"{{.*}}/Inputs/funcimport.ll"}
----------------
This stat is not labeled correctly. It at least needs to change to the number of globals imported. It would be nice to have separate stats for functions and variables imported though.

Please also fix the debug import dumping code in dumpImportListForModule and at the end of ComputeCrossModuleImport to report functions and variables separately.


https://reviews.llvm.org/D43077