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

[apazos]

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.

[apazos]

Adding Owen to the review.

[resistor]

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

[apazos]

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]

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]

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]

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]

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.

2. 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]

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

[hfinkel]

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]

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]

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

[Eugene.Zelenko]

Looks like patch was not committed.

[apazos]

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

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

bool DAGCombiner::MergeConsecutiveStores(StoreSDNode* St) {
 
   return true;
 }
 
 SDValue DAGCombiner::replaceStoreChain(StoreSDNode *ST, SDValue BetterChain) {
   SDLoc SL(ST);
   SDValue ReplStore;
 
   // Replace the chain to avoid dependency.
-  if (ST->isTruncatingStore()) {
+  if (ST->isTruncatingStore())
     ReplStore = DAG.getTruncStore(BetterChain, SL, ST->getValue(),
                                   ST->getBasePtr(), ST->getMemoryVT(),
                                   ST->getMemOperand());
-  } else {
+  else
     ReplStore = DAG.getStore(BetterChain, SL, ST->getValue(), ST->getBasePtr(),
                              ST->getMemOperand());
+
+  unsigned Chain = (ST->getNumValues() > 1) ? 1 : 0;
+
+  if (Chain) {
+    SDValue Base = ST->getBasePtr();
+    ISD::MemIndexedMode AM = ST->getAddressingMode();
+    SDValue Offset = ST->getOperand(3);
+    ReplStore = DAG.getIndexedStore(ReplStore, SL, Base, Offset, AM);
   }
 
   // Create token to keep both nodes around.
   SDValue Token = DAG.getNode(ISD::TokenFactor, SL,
-                              MVT::Other, ST->getChain(), ReplStore);
+                              MVT::Other, ST->getChain(),
+                              ReplStore.getValue(Chain));
 
   // Make sure the new and old chains are cleaned up.
   AddToWorklist(Token.getNode());
 
   // Don't add users to work list.
+  if (Chain)
+    return CombineTo(ST, ReplStore, Token, false);
   return CombineTo(ST, Token, false);
 }
 
 SDValue DAGCombiner::replaceStoreOfFPConstant(StoreSDNode *ST) {
   SDValue Value = ST->getValue();
   if (Value.getOpcode() == ISD::TargetConstantFP)
     return SDValue();