Page MenuHomePhabricator

[SDAG] Relax conditions under stores of loaded values can be merged
Needs ReviewPublic

Authored by niravd on Feb 28 2017, 1:15 PM.

Details

Summary

Allow consecutive stores whose values come from consecutive loads to
merged in the presense of other uses of the loads. Previously this was
disallowed as in general the merged load cannot be shared with the
other uses. Merging N stores into 1 may cause as many as N redundant
loads. However in the context of caching this should have neglible
affect on memory pressure and reduce instruction count making it
almost always a win.

Fixes PR32086.

Diff Detail

Event Timeline

niravd created this revision.Feb 28 2017, 1:15 PM
spatel edited edge metadata.

The output certainly looks good - thanks!

I'm still not confident in my understanding of chains and tokenfactors, so adding some more potential reviewers. I just made some minor comments for the test file.

test/CodeGen/X86/merge_store_duplicated_loads.ll
1–4 ↗(On Diff #90077)
  1. Can you please commit these tests before the code change? That way we have a baseline check for today's codegen, and we'll just see the improvements from your patch.
  1. If you put the triple in the RUN line, you can use utils/update_llc_test_checks.py to auto-generate the check lines. Then, when you update the tests with the code change, you just run that script again and won't have to manually write any CHECK lines.
  1. Please add a comment with 'PR32086' and/or a link to bugzilla here. That makes it easier to keep track of the motivation and backstory for the tests.
efriedma added inline comments.Mar 1 2017, 11:55 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12015 ↗(On Diff #90077)

Is this a correctness check, or a profitability check? At first glance, it isn't obvious why we're checking any of this here.

12414 ↗(On Diff #90077)

Why do you need to CombineTo twice? Please explain in a comment.

niravd updated this revision to Diff 90277.Mar 1 2017, 8:07 PM
niravd marked an inline comment as done.

Add comments and do minor cleanup of parameter filter.

niravd marked an inline comment as done.Mar 1 2017, 8:24 PM
niravd added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12015 ↗(On Diff #90077)

Folded in some pending code cleanup that makes this a bit mroe clear. This is a filter to make sure we only get mergeable candidates that would be profitable.

efriedma added inline comments.Mar 2 2017, 2:58 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12015 ↗(On Diff #90077)

Okay, that's better.

12430 ↗(On Diff #90277)

Ah, I see what you're doing. I really don't like the fact that you're creating a TokenFactor which uses itself, even temporarily... but I'm not sure what the best approach is. Could you ask on llvmdev?

niravd updated this revision to Diff 92336.Mar 20 2017, 9:00 AM

Cleanup and rebase over D31068

niravd retitled this revision from [DAG] Relax conditions under stores of loaded values can be merged to [SDAG] Relax conditions under stores of loaded values can be merged.Mar 20 2017, 9:00 AM
niravd edited the summary of this revision. (Show Details)
niravd updated this revision to Diff 95584.Apr 18 2017, 9:11 AM
niravd edited the summary of this revision. (Show Details)

Rebasing to head to show simplified patch.

niravd updated this revision to Diff 98294.May 9 2017, 8:23 AM

Rewrite chain update to address Eli's concerns.

efriedma added inline comments.May 9 2017, 11:11 AM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
12941 ↗(On Diff #98294)

The approach makes sense... but I'm surprised a TokenFactor with zero operands doesn't cause an assertion failure somewhere. Could you use a new opcode for this?

Also, please add an assertion that TokenDummy has zero uses.

niravd updated this revision to Diff 98444.May 10 2017, 6:49 AM
niravd marked an inline comment as done.

Address Eli's comments

This revision is now accepted and ready to land.May 10 2017, 11:08 AM
This revision was automatically updated to reflect the committed changes.
niravd reopened this revision.May 21 2017, 6:06 AM

Temporarily reverted due to error in clang-ppc64be-linux-multistage.

This revision is now accepted and ready to land.May 21 2017, 6:06 AM
RKSimon added inline comments.
llvm/trunk/test/CodeGen/X86/merge_store_duplicated_loads.ll
5 ↗(On Diff #98511)

As mentioned on PR32086, please remove this triple and add a -mtriple=i686-unknown-linux-gnu test run to check 32-bit codegen as well.

niravd updated this revision to Diff 99791.May 22 2017, 12:20 PM

Rebase past StoreMerge Cleanup.

niravd updated this revision to Diff 102187.Jun 12 2017, 8:38 AM

PPC bug exists, but rebasing to make use of Sanjay's helper function cleanup.

RKSimon requested changes to this revision.Nov 7 2017, 4:01 AM

Reopening this until the PPC regressions are fixed

This revision now requires changes to proceed.Nov 7 2017, 4:01 AM

Not sure if the PPC issue was a different one, but the commit that reverted this was:

Revert "[SDAG] Relax conditions under stores of loaded values can be merged"

This reverts r302712.

The change fails with ASAN enabled:

ERROR: AddressSanitizer: use-after-poison on address ... at ...
READ of size 2 at ... thread T0
  #0 ... in llvm::SDNode::getNumValues() const <snip>/include/llvm/CodeGen/SelectionDAGNodes.h:855:42
  #1 ... in llvm::SDNode::hasAnyUseOfValue(unsigned int) const <snip>/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:7270:3
  #2 ... in llvm::SDValue::use_empty() const <snip> include/llvm/CodeGen/SelectionDAGNodes.h:1042:17
  #3 ... in (anonymous namespace)::DAGCombiner::MergeConsecutiveStores(llvm::StoreSDNode*) <snip>/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12944:7

Reviewers: niravd

Subscribers: llvm-commits

Differential Revision: https://reviews.llvm.org/D33081

llvm-svn: 302746
This revision was not accepted when it landed; it landed in state Needs Revision.Oct 7 2019, 6:06 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 6:06 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon reopened this revision.Oct 7 2019, 6:10 AM
RKSimon requested changes to this revision.
This revision now requires changes to proceed.Oct 7 2019, 6:10 AM
This revision was not accepted when it landed; it landed in state Needs Revision.Oct 7 2019, 6:20 AM
This revision was automatically updated to reflect the committed changes.
efriedma reopened this revision.Oct 7 2019, 12:47 PM
niravd added a comment.EditedOct 7 2019, 1:01 PM

I can't speak to the PPC issue, but from jyknight's comment:

This is almost certainly just a matter of deleting the assert. I would land this myself, but I've yet to go through the official "you can commit" with the new companieslegal.

Is anyone willing to try removing the assert and recommiting?

@niravd What's the status of this patch?

niravd added a comment.Nov 4 2019, 8:48 AM

I've not touched this and probably will not for the forseeable future. Is someone else willing to take the lead on this?

@niravd What's the status of this patch?