[PATCH] D17082: FunctionImport: add a progressive heuristic to limit importing too deep in the callgraph

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

This is a reasonable heuristic until we have more sophisticated cost/benefit analysis, especially with PGO data. However, there is an issue due to the depth first traversal order, see below.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:268
@@ -256,1 +267,3 @@
+    // Adjust the threshold
+    Threshold = Threshold * ImportInstrFactor;
     F->materialize();
----------------
I think we can get missed importing opportunities due to the depth-first nature of the current traversal. We push_back newly identified external calls (which have the reduced threshold), then pop_back_val when traversing the Worklist above, resulting in a depth-first traversal of the call graph. What this means is that if we first encounter a call to a given external function foo() deep in a call chain, it will get added with a very low threshold. It may be that another external call to foo() is encountered higher up the call chain in something added to the worklist earlier and popped later, but since we skip any functions that were already seen (in the CalledFunctions set), we will miss the opportunity to add it with the higher threshold and possibly lose out on the import.

To fix this the tracking of what is or was already in the worklist needs to be more sophisticated. Perhaps a simple fix would be to change the CalledFunctions set to a map from function name to the max threshold it was added to the worklist with. Then if we encounter another call to the same function, if it currently has a higher threshold than recorded in the CalledFunctions map, add it to the worklist again and updated the map entry. Then each function popped off the worklist should be compared against the existing ModuleToFunctionsToImportMap entry to see if we have already decided to import it since it may end up in the worklist multiple times.


http://reviews.llvm.org/D17082