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

[Nirav Dave via Phabricator via llvm-commits]

niravd added a comment.



> I havn't got the idea. Could you please elaborate what is the "SDNode *" in the key and what is the "SDNode *" in the value? And how it helps the early return in the dependence check?

Not sure if you mean the first or second point, but here's both:

1. The issue with having the values incremented in the candidate search is that we'll be much more aggressive about marking things non-mergable. There are a number of cases where we need to visit each store in the merge set once before it can be merged as part dag combining the last store. Also, by deferring the increment to when checkMergeStoreCandidatesForDependencies fails, store merge slowdowns unrelated to this will continue to appear as compile time issues not performance degradations which means are much more likely to consider them when they come up.

2. Write the map as Candidate Store to (RootNode, count) where the root node acts as "a checksum". So the dependence check is lookup and match checksum and the update is lookup, increment on checksum match and reset to 1 on mismatch.



>> 
>> 
>> 3. The RootStoreCountMap ends up with stale information as we replace nodes, which may cause unexpected failures. Can you modify the node listeners to trim the map.
> 
> Good point. I will update it.




Repository:
  rL LLVM

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

https://reviews.llvm.org/D65174