[PATCH] D53254: [Merge SImilar Function ThinLTO 5/n] Set up similar function to be imported
Teresa Johnson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 15 10:22:06 PDT 2018
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
More information about the llvm-commits
mailing list