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

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya 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())
----------------
tejohnson wrote:
> 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.
Will do that. Thanks for pointing out.


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


================
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())
----------------
tejohnson wrote:
> Why this constraint? Note that any global variables being referenced by an imported function will automatically have their declarations imported too.
I'll remove this comment. Does not seem to be relevant.


https://reviews.llvm.org/D53254