[PATCH] D16463: [DAGCombiner] Check store node's value count before replaceStoreChain

[Junmo Park via llvm-commits]

flyingforyou added a comment.

Thanks for comment Chad.

I respect your opinion. But I couldn't add testcase for this patch.
Because it's about assert.

So, I want to explain more about this.

  for (StoreSDNode *ChainedStore : ChainedStores) {
    // If ChainedStore has more than one value, we don't try replaceStoreChain.
    // replaceStoreChain uses CombineTo, which only consider Chain only.
    if (ChainedStore->getNumValues() != 1)
      continue;
  
    SDValue Chain = ChainedStore->getChain();
    SDValue BetterChain = FindBetterChain(ChainedStore, Chain);
  
    if (Chain != BetterChain) {
      MadeChange = true;
      BetterChains.push_back(std::make_pair(ChainedStore, BetterChain));
    }
  }
  
  // Do all replacements after finding the replacements to make to avoid making
  // the chains more complicated by introducing new TokenFactors.
  for (auto Replacement : BetterChains)
    replaceStoreChain(Replacement.first, Replacement.second);

When you see inside of for loop, `ChainedStore` is added to `BetterChains`.
After this, `replaceStoreChain` get `ChainedStore` for first parameter.

  SDValue DAGCombiner::replaceStoreChain(StoreSDNode *ST, SDValue BetterChain) {
    SDLoc SL(ST);
    SDValue ReplStore;
  
    // Replace the chain to avoid dependency.
    if (ST->isTruncatingStore()) {
      ReplStore = DAG.getTruncStore(BetterChain, SL, ST->getValue(),
                                    ST->getBasePtr(), ST->getMemoryVT(),
                                    ST->getMemOperand());
    } else {
      ReplStore = DAG.getStore(BetterChain, SL, ST->getValue(), ST->getBasePtr(),
                               ST->getMemOperand());
    }
  
    // Create token to keep both nodes around.
    SDValue Token = DAG.getNode(ISD::TokenFactor, SL,
                                MVT::Other, ST->getChain(), ReplStore);
  
    // Make sure the new and old chains are cleaned up.
    AddToWorklist(Token.getNode());
  
    // Don't add users to work list.
    return CombineTo(ST, Token, false);
  }

Inside of `replaceStoreChain`, `ChainedStore` will be passed to `CombineTo`'s first parameter.
And Token which is `CombineTo`'s second paratemter only can be combined with same type node.

  SDValue DAGCombiner::CombineTo(SDNode *N, const SDValue *To, unsigned NumTo,
                                 bool AddTo) {
    assert(N->getNumValues() == NumTo && "Broken CombineTo call!");
    ++NodesCombined;
    DEBUG(dbgs() << "\nReplacing.1 ";
          N->dump(&DAG);
          dbgs() << "\nWith: ";
          To[0].getNode()->dump(&DAG);
          dbgs() << " and " << NumTo-1 << " other values\n");
    for (unsigned i = 0, e = NumTo; i != e; ++i)
      assert((!To[i].getNode() ||
              N->getValueType(i) == To[i].getValueType()) &&
             "Cannot combine value to value of different type!");
  
    WorklistRemover DeadNodes(*this);
    DAG.ReplaceAllUsesWith(N, To);
    if (AddTo) {
      // Push the new nodes and any users onto the worklist
      for (unsigned i = 0, e = NumTo; i != e; ++i) {
        if (To[i].getNode()) {
          AddToWorklist(To[i].getNode());
          AddUsersToWorklist(To[i].getNode());
        }
      }
    }
  
    // Finally, if the node is now dead, remove it from the graph.  The node
    // may not be dead if the replacement process recursively simplified to
    // something else needing this node.
    if (N->use_empty())
      deleteAndRecombine(N);
    return SDValue(N, 0);
  }

There is a assert check  `assert(N->getNumValues() == NumTo && "Broken CombineTo call!");`.
When we call `CombineTo(ST, Token, false)`, this is changed likes below.

  /// Replaces all uses of the results of one DAG node with new values.
  SDValue CombineTo(SDNode *N, SDValue Res, bool AddTo = true) {
    return CombineTo(N, &Res, 1, AddTo);
  }

That means `NumTo` is always `1`. This is why I added check code likes below.

  if (ChainedStore->getNumValues() != 1)
    continue;

Junmo.


http://reviews.llvm.org/D16463