Cleanup Store Merging in UseAA case

[niravd]

Summary

This patch fixes a bug (PR26827) when using anti-aliasing in store merging.
This sets the chain users of the component stores to point to the new merged store
instead of the component stores chain parent.

[mgrang]

Can you add a unit test for this?

[niravd]

In D18909#395936, @mgrang wrote:

Can you add a unit test for this?

Not one that highlights the difference. This is a factoring of some of the factorable cleanup of D14834. D14834 will remove the old path for store merging.

[jyknight]

Not one that highlights the difference. This is a factoring of some of the factorable cleanup of D14834. D14834 will remove the old path for store merging.

You ought to be able to detect that the chain is being set correctly now and wasn't before -- at the very least by looking at the nodes being produced.

Does this change by itself fix the test case in http://reviews.llvm.org/D17836?

[niravd]

In D18909#396963, @jyknight wrote:

Not one that highlights the difference. This is a factoring of some of the factorable cleanup of D14834. D14834 will remove the old path for store merging.

You ought to be able to detect that the chain is being set correctly now and wasn't before -- at the very least by looking at the nodes being produced.

Does this change by itself fix the test case in http://reviews.llvm.org/D17836?

Ah. So this isn't just a clean up, but actually fixing the UseAA case of merging. You're intuition is right here James, the test from D17836 case does work. I'll update the diff.

[niravd]

Add test case

[mcrosier]

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

11232

Please add a period.

[jyknight]

Added cc's from D17836 which this obsoletes.

[jyknight]

LGTM (after fixing period as per other comment, of course).

[niravd]

Add test case and period

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

bool DAGCombiner::MergeStoresOfConstantsOrVecElts(
 
   SDValue NewChain = DAG.getNode(ISD::TokenFactor, DL, MVT::Other, Chains);
   SDValue NewStore = DAG.getStore(NewChain, DL, StoredVal,
                                   FirstInChain->getBasePtr(),
                                   FirstInChain->getPointerInfo(),
                                   false, false,
                                   FirstInChain->getAlignment());
 
+  bool UseAA = CombinerAA.getNumOccurrences() > 0 ? CombinerAA
+                                                  : DAG.getSubtarget().useAA();
+  if(UseAA){
+    // Replace all merged stores with the new store
+    for (unsigned i = 0; i < NumStores; ++i)
+      CombineTo(StoreNodes[i].MemNode, NewStore);
+  }
+  else{
     // Replace the last store with the new store
     CombineTo(LatestOp, NewStore);
     // Erase all other stores.
     for (unsigned i = 0; i < NumStores; ++i) {
       if (StoreNodes[i].MemNode == LatestOp)
         continue;
       StoreSDNode *St = cast<StoreSDNode>(StoreNodes[i].MemNode);
       // ReplaceAllUsesWith will replace all uses that existed when it was
       // called, but graph optimizations may cause new ones to appear. For
       // example, the case in pr14333 looks like
       //
       //  St's chain -> St -> another store -> X
       //
       // And the only difference from St to the other store is the chain.
       // When we change it's chain to be St's chain they become identical,
       // get CSEed and the net result is that X is now a use of St.
       // Since we know that St is redundant, just iterate.
       while (!St->use_empty())
         DAG.ReplaceAllUsesWith(SDValue(St, 0), St->getChain());
       deleteAndRecombine(St);
     }
-
+  }
   return true;
 }
 
 void DAGCombiner::getStoreMergeAndAliasCandidates(
     StoreSDNode* St, SmallVectorImpl<MemOpLink> &StoreNodes,
     SmallVectorImpl<LSBaseSDNode*> &AliasLoadNodes) {
   // This holds the base pointer, index, and the offset in bytes from the base
   // pointer.
bool DAGCombiner::MergeConsecutiveStores(StoreSDNode* St) {
 
   // Transfer chain users from old loads to the new load.
   for (unsigned i = 0; i < NumElem; ++i) {
     LoadSDNode *Ld = cast<LoadSDNode>(LoadNodes[i].MemNode);
     DAG.ReplaceAllUsesOfValueWith(SDValue(Ld, 1),
                                   SDValue(NewLoad.getNode(), 1));
   }
 
+  if (UseAA){
+    // Replace the all stores with the new store.
+    for (unsigned i = 0; i < NumElem; ++i)
+      CombineTo(StoreNodes[i].MemNode, NewStore);
+  }
+  else{
     // Replace the last store with the new store.
     CombineTo(LatestOp, NewStore);
     // Erase all other stores.
     for (unsigned i = 0; i < NumElem ; ++i) {
       // Remove all Store nodes.
       if (StoreNodes[i].MemNode == LatestOp)
         continue;
       StoreSDNode *St = cast<StoreSDNode>(StoreNodes[i].MemNode);
       DAG.ReplaceAllUsesOfValueWith(SDValue(St, 0), St->getChain());
       deleteAndRecombine(St);
     }
+  }
 
   return true;
 }
 
 SDValue DAGCombiner::replaceStoreChain(StoreSDNode *ST, SDValue BetterChain) {
   SDLoc SL(ST);
   SDValue ReplStore;