Page MenuHomePhabricator

[SDAG] Expand MergedConsecutiveStores to better handle Giving up in Chain Analysis
AbandonedPublic

Authored by niravd on Mar 16 2017, 8:19 PM.

Details

Summary

For large blocks of code, FindBetterChain will give up leaving memory operations in their initial configuration. A common idiom for this results in a long chain of stores which are generally mergable. Augment the search to consider these stores as merge candidates.

Diff Detail

Event Timeline

niravd created this revision.Mar 16 2017, 8:19 PM
niravd updated this revision to Diff 92331.Mar 20 2017, 8:26 AM

Add testcase and minor cleanup.

niravd retitled this revision from [SDAG] Expand MergedConsecutiveStores to handle store chains to [SDAG] Expand MergedConsecutiveStores to better handle Giving up in Chain Analysis.Apr 10 2017, 6:41 AM
niravd edited the summary of this revision. (Show Details)

Hi Nirav
I tried reproducing it with the patch and the issue is still not fixed. I'll try to get back with a repro case (for open source targets).

Nirav - I posted the reason for the regression and a quick fix for the issue I saw in https://reviews.llvm.org/D31914.

I'm a little concerned about the selection candidates having a chain loop dependence after mething. Can you get a llvm testcase I can look at myself. Alternatively I can probably cobble one myself if I can get a snapshot of the DAG right before the relevant merge

Nirav - I posted the reason for the regression and a quick fix for the issue I saw in https://reviews.llvm.org/D31914.

niravd updated this revision to Diff 97480.May 2 2017, 11:28 AM
niravd edited the summary of this revision. (Show Details)

Rebasing.

courbet added inline comments.Oct 16 2018, 4:02 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12413 ↗(On Diff #97480)

Please rebase, you'll have to go through CandidateMatch's Offset parameter now.

test/CodeGen/X86/stores-merging2.ll
16 ↗(On Diff #97480)

This test fails to fail before this change when rebased on master. I guess this is because the value of GatherAllAliasesMaxDepth has been changed from 6 to 18.

The test quite brittle against that kind of config changes BTW. You should at least leave a comment for the future explaining how to fix the test if changing the value.

niravd updated this revision to Diff 170032.Oct 17 2018, 1:01 PM

Rebase to head. Some changes to alias make this require a much longer chain of stores. The need is clearly less than when I wrote this patch a year ago, but still has a minor positive effect.

Rebase to head. Some changes to alias make this require a much longer chain of stores. The need is clearly less than when I wrote this patch a year ago, but still has a minor positive effect.

Do you think likely to happen in practice ?

It's certainly less likely than before, but it does happen in the wild, but D53552 should always parallelize these store chains, so this this should be droppable.

Rebase to head. Some changes to alias make this require a much longer chain of stores. The need is clearly less than when I wrote this patch a year ago, but still has a minor positive effect.

Do you think likely to happen in practice ?

Rebase to head. Some changes to alias make this require a much longer chain of stores. The need is clearly less than when I wrote this patch a year ago, but still has a minor positive effect.

Do you think likely to happen in practice ?

niravd abandoned this revision.Nov 9 2018, 8:19 AM

This is no longer necessary now that r346432 has landed.