[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 a comment.

Thanks for the suggestions. I will update patch immediately. Please see if it matches what is in your mind.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:118
 
+static cl::opt<unsigned> LimitInDependenceCheck(
+    "limit-in-dependence-check", cl::Hidden, cl::init(100),
----------------
niravd wrote:
> the command flag name is a bit unclear and should have the "combiner-" prefix. Maybe "combiner-store-merge-dependence-limit"?
Done. 


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15481
           if (CandidateMatch(OtherST, Ptr, PtrDiff))
             StoreNodes.push_back(MemOpLink(OtherST, PtrDiff));
         }
----------------
niravd wrote:
> No sense in considering stores that won't merge. Filter candidates on StoreRootCountMap before we add to StoreNodes here. 
Done.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15491
 bool DAGCombiner::checkMergeStoreCandidatesForDependencies(
     SmallVectorImpl<MemOpLink> &StoreNodes, unsigned NumStores,
+    SDNode *RootNode, SDNode *St) {
----------------
niravd wrote:
> If we're going to fail from dependences for an N store merge, we encounter it for a merge starting from each of the N candidates. We should 
> mark each of the "NumStores" candidate stores we have when we fail. Also, you shouldn't bother updating unless the dependence check failed from timeout. 
> 
Good point. Done. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65174