[PATCH] D65174: [DAGCombine] Limit the number of times for a store being considered for merging

[Wei Mi via Phabricator via llvm-commits]

wmi created this revision.
wmi added reviewers: RKSimon, spatel.
Herald added a subscriber: hiraditya.
Herald added a project: LLVM.

Recently we run into a case with compile time problem (27 mins in O3 <https://reviews.llvm.org/owners/package/3/> but a few seconds in O0). In that case, we have a huge block containing many stores. Each time a store is visited, dagcombiner will find a candidate set all chained by the same root node for the store, then try to merge the nodes in the candidate set into a wider store. This is a quadratic algorithm, because for the same candidate set, everytime a store inside of the set is visited, the same set will be considered for merging. 

In this patch, we add a map to record how many times the pair of root node and the store node is seen before a store node is added into candidate set. If the count is above a threshold (set to 10 by default now), the store node won't be added to candidate set again. With the patch, the compile time drops to 20 seconds.

I verify that the patch won't miss store merging opportunities in most cases in the way that by building a large internal server application and bootstrapping clang w/wo the patch, I find no difference for the binaries.


Repository:
  rL LLVM

https://reviews.llvm.org/D65174

Files:
  llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp


Index: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
+++ llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
@@ -115,6 +115,11 @@
     "combiner-tokenfactor-inline-limit", cl::Hidden, cl::init(2048),
     cl::desc("Limit the number of operands to inline for Token Factors"));
 
+static cl::opt<unsigned> StoreMergeLimit(
+    "store-merge-limit", cl::Hidden, cl::init(10),
+    cl::desc("Limit the number of times for a store being considered for "
+             "merging from the same root. "));
+
 namespace {
 
   class DAGCombiner {
@@ -152,6 +157,11 @@
     /// which have not yet been combined to the worklist.
     SmallPtrSet<SDNode *, 32> CombinedNodes;
 
+    /// Store how many times StoreNode is added to store merge candidates
+    /// because it is chained to RootNode. This is used to prevent a StoreNode
+    /// from being considered for merging too many times for very large Block.
+    DenseMap<std::pair<SDNode *, SDNode *>, unsigned> RootStorePairCountMap;
+
     // AA - Used for DAG load/store alias analysis.
     AliasAnalysis *AA;
 
@@ -15446,6 +15456,11 @@
   unsigned NumNodesExplored = 0;
   if (LoadSDNode *Ldn = dyn_cast<LoadSDNode>(RootNode)) {
     RootNode = Ldn->getChain().getNode();
+    // Limit the number of times for a store being considered for
+    // merging from the same root.
+    if (RootStorePairCountMap[{RootNode, St}] > StoreMergeLimit)
+      return;
+
     for (auto I = RootNode->use_begin(), E = RootNode->use_end();
          I != E && NumNodesExplored < 1024; ++I, ++NumNodesExplored)
       if (I.getOperandNo() == 0 && isa<LoadSDNode>(*I)) // walk down chain
@@ -15454,19 +15469,29 @@
             if (StoreSDNode *OtherST = dyn_cast<StoreSDNode>(*I2)) {
               BaseIndexOffset Ptr;
               int64_t PtrDiff;
-              if (CandidateMatch(OtherST, Ptr, PtrDiff))
+              if (CandidateMatch(OtherST, Ptr, PtrDiff)) {
+                RootStorePairCountMap[{RootNode, OtherST}]++;
                 StoreNodes.push_back(MemOpLink(OtherST, PtrDiff));
+              }
             }
-  } else
+  } else {
+    // Limit the number of times for a store being considered for
+    // merging from the same root.
+    if (RootStorePairCountMap[{RootNode, St}] > StoreMergeLimit)
+      return;
+
     for (auto I = RootNode->use_begin(), E = RootNode->use_end();
          I != E && NumNodesExplored < 1024; ++I, ++NumNodesExplored)
       if (I.getOperandNo() == 0)
         if (StoreSDNode *OtherST = dyn_cast<StoreSDNode>(*I)) {
           BaseIndexOffset Ptr;
           int64_t PtrDiff;
-          if (CandidateMatch(OtherST, Ptr, PtrDiff))
+          if (CandidateMatch(OtherST, Ptr, PtrDiff)) {
+            RootStorePairCountMap[{RootNode, OtherST}]++;
             StoreNodes.push_back(MemOpLink(OtherST, PtrDiff));
+          }
         }
+  }
 }
 
 // We need to check that merging these stores does not cause a loop in


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D65174.211379.patch
Type: text/x-patch
Size: 3019 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190723/2d2a0421/attachment.bin>