Page MenuHomePhabricator

[DAGCombiner] Handle pre/post-inc stores in replaceStoreChain
AbandonedPublic

Authored by apazos on Dec 8 2015, 1:53 PM.

Details

Summary

The code in DAGCombiner::replaceStoreChain and CombineTo do not properly handle store operations with pre/post increment.

This causes assertion in CombineTo call: assert(N->getNumValues() == NumTo && "Broken CombineTo call!"). In addition, replaceStoreChain might replace a store with pre-increment with an unindexed store which accesses the wrong base ptr, causing incorrect code to be generated.

Diff Detail

Repository
rL LLVM

Event Timeline

apazos updated this revision to Diff 42216.Dec 8 2015, 1:53 PM
apazos retitled this revision from to [DAGCombiner] Skip pre/post-inc stores in findBetterNeighborChains.
apazos updated this object.
apazos added a comment.Dec 8 2015, 1:55 PM

Adding Owen to the review.

resistor edited edge metadata.Dec 10 2015, 1:01 PM

Your proposed fix seems conservatively correct to me. I'd recommend getting a review from one of the ARM maintainers, perhaps tnorthover?

Hi Tim,

Do you have another suggestion for fixing this issue?

DAGCombiner::replaceStoreChain with try to run CombineTo with a Token node (always one result) with a store operation that can have more than one result (if it has pre/post inc).

apazos updated this revision to Diff 42615.Dec 11 2015, 6:12 PM
apazos edited edge metadata.

Instead of skipping store with pre/post inc instructions, I inlined CombineTo and fixed the code to handle stoer with pre/post inc correctly without causing the assertion. Let me know what you think.

zzheng added a subscriber: zzheng.Dec 12 2015, 6:10 PM
zzheng added inline comments.
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11756

Inlining/Specializing CombineTo() here would cause extra maintenance overhead.

Can we change CombineTo() to handle such special cases?

apazos added inline comments.Dec 14 2015, 12:27 PM
lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11756–11758

There are other instances in the code where we are already doing similar inlining (see CommitTargetLoweringOpt() in the same file) because we want to replace uses of a particular value while leaving the other values untouched (use of ReplaceAllUsesOfValueWith instead of ReplaceAllUsesWith).

apazos updated this revision to Diff 44267.Jan 7 2016, 3:22 PM

I investigated further and there are two issues in SDValue DAGCombiner::replaceStoreChain:

  1. CombineTo will assert if the store node passed in is a pre/post inc store.

It will not match the type of Token node because of the Chain + updated base ptr results.

  1. The new store created is not an indexed store when the store node passed in is a pre/post inc store.

So in my failing tests I see a replacement store to a wrong location which is causing crashes.

I decided to then create indexed stores that match the type of the store node passed in and fixed the CombineTo calls.

apazos added a subscriber: hfinkel.Jan 7 2016, 3:24 PM
apazos retitled this revision from [DAGCombiner] Skip pre/post-inc stores in findBetterNeighborChains to [DAGCombiner] Handle pre/post-inc stores in replaceStoreChain.Jan 12 2016, 4:28 PM
apazos updated this object.

Can someone help review this change please? I would like to check in this bug fix.

Do you have any test cases for this change?

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11730

The '// Chain' does not add anything here.

11738

Why is this code here? Can't you have a pre/post-inc truncating store (I'm pretty sure that you can). Seems like it should be outside this 'else' block.

11742
SDValue Offset = ST->getOffset();
11745

Why do you specifically delete these nodes here? Dead nodes are normally cleaned up automatically once their use counts drop to zero (DAGCombiner::Run calls recursivelyDeleteUnusedNodes).

apazos updated this revision to Diff 44777.Jan 13 2016, 12:23 PM
apazos updated this object.
apazos set the repository for this revision to rL LLVM.

Hi Hal, thanks for looking at this, I have updated the code.

The failing scenario comes from a LTO config and it is hard to produce a reduced test case.

But code inspection shows the issues too: if the store instr being passed in is indexed
(1) the original CombineTo call will assert because Token and original store instr will have different number of result operands.
(2) The replacement store won't be indexed and we will have a new store operation that will read from the wrong base ptr, if the original store has pre-increment (which is the case in my internal LTO test failure).

hfinkel accepted this revision.Feb 2 2016, 6:31 PM
hfinkel added a reviewer: hfinkel.

Aside from the getOffset substitution, LGTM. When you commit, please specifically note why you don't have a reduced test case.

As a general note, it seems like we need to make it easier to use bugpoint on cases that come from LTO. This issue that it is difficult to reduce LTO crashes seems to come up a lot and it quite unfortunate.

lib/CodeGen/SelectionDAG/DAGCombiner.cpp
11742

Please use ST->getOffset() instead of ST->getOperand(3).

This revision is now accepted and ready to land.Feb 2 2016, 6:31 PM

Looks like patch was not committed.

In https://reviews.llvm.org/D16463, the check for indexed, prevents this failure.

apazos abandoned this revision.Mar 26 2018, 3:06 PM