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

[Wei Mi via Phabricator via llvm-commits]

wmi added a comment.

In D65174#1606368 <https://reviews.llvm.org/D65174#1606368>, @niravd wrote:

> > It may not be easy for us to prune the searching space without missing some merging opportunity. I feel it is easier to achieve that goal by controlling how many times a {RootNode, StoreNode} pair appear in candidate set repeatedly. With the patch, no bit change in the binaries were found for clang and some large application. So the current patch is a safe change.
>
> True. But my concern is that as we prune the search space, we will introduce burrs in the which will be extremely hard to find and notice. I agree that its' not worth it to solve it the ideal way, but if a quick to write but safe pruning of the search space happens to work we should go with that. If not, it's likely it'll be too hard to resolve better than a heuristics. The scheme here is fine modulo the following concerns:
>
> 1. If it's really an issue with the candidate dependence check, we should move the RootStoreCountMap update be modified in the dependency search, not the candidate check. This shouldn't dramatically change the run time.
> 2. The RootNode is effectively a function of the candidate store. We really should only have a single RootNode  so it'd be better as a map from SDNode* to (SDNode*, int).


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?

> 
> 
> 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