[PATCH] D53254: [Merge SImilar Function ThinLTO 5/n] Set up similar function to be imported

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:488
+  // TODO: Make hashing little more strict to have higher hit rate
+  auto &HostSimilarFunctions = Index.getHostSimilarFunction();
+  if (HostSimilarFunctions.empty())
----------------
brzycki wrote:
> ooh boy this is a dense section of code. Possibly consider refactoring into helper functions for readability?
This is a good suggestion. Also, some high level comments describing what this code is doing/how it works would be helpful.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:500
+    if (SimHash != SimHashFromIndex) {
+      LLVM_DEBUG(dbgs() << "Assertion failed: (SimHash == SimHashFromIndex);");
+      return;
----------------
Seems like this should be an actual assert. Actually, there is such an assert below, why the early return - leftover from debugging?


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:508
+      // Import GUIDs from outside module.
+      // FIXME: Import a function once ever!
+      // assert if a GUID is exported twice
----------------
You should fix this, and it should be straightforward. See where we check the return value of the ImportList insertion for importing above (variable PreviouslyImported).


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:512
+        // Get the Summary from GUID to add to export summary.
+        if (ValueInfo VI = Index.getValueInfo(GUID)) {
+          if (VI.getSummaryList().size() == 1) {
----------------
This could be avoided (and save an expensive hash lookup), if the SimFunctions saved the VIs to start with (although I haven't yet looked through your other patches to see how this is being populated).


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:519
+            auto FS = cast<FunctionSummary>(S);
+            // TODO: Import declarations of global variables as well.
+            if (FS->refs().size())
----------------
Why this constraint? Note that any global variables being referenced by an imported function will automatically have their declarations imported too.


https://reviews.llvm.org/D53254