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

[Wei Mi via Phabricator via llvm-commits]

wmi marked 3 inline comments as done.
wmi added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAGNodes.h:869
+  /// whether the searching bails out.
   static bool hasPredecessorHelper(const SDNode *N,
                                    SmallPtrSetImpl<const SDNode *> &Visited,
----------------
niravd wrote:
> Visited.size() and maxsteps are in scope for any call of the help. Just do the comparision at the call point instead of adding an argument. 
Ah, I didn't notice that. Will change. 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15446
+                                        SDNode *RootNode) -> bool {
+    return false;
+    auto RootCount = StoreRootCountMap.find(StoreNode);
----------------
niravd wrote:
> Extraneous return false here.
Thanks for spotting it. I add it for experiment but forget to remove it when generating the patch. 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15560
+      if (BailOut) {
+        auto &RootCount = StoreRootCountMap[StoreNodes[i].MemNode];
+        RootCount = {RootNode, RootCount.second + 1};
----------------
niravd wrote:
> Shouldn't you be checking that RootNode matches before you increment the count?
Yes, I should. Done.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65174/new/

https://reviews.llvm.org/D65174