[PATCH] D53552: [DAGCombine] Improve alias analysis for chain of independent stores.

[Clement Courbet via Phabricator via llvm-commits]

courbet added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18944
 // fully-merged state.
 bool DAGCombiner::findBetterNeighborChains(StoreSDNode *St) {
   if (OptLevel == CodeGenOpt::None)
----------------
I think readability would be much better if this was split a bit:

```
bool DAGCombiner::findBetterNeighborChains(StoreSDNode *St) {
  if (OptLevel == CodeGenOpt::None)
    return false;

  // This holds the base pointer, index, and the offset in bytes from the base
  // pointer.
  BaseIndexOffset BasePtr = BaseIndexOffset::match(St, DAG);

  // We must have a base and an offset.
  if (!BasePtr.getBase().getNode())
    return false;

  // Do not handle stores to undef base pointers.
  if (BasePtr.getBase().isUndef())
    return false;
  
  // First try to merge chained stores.
  StoreSDNode *STChain = St;
  SmallVector<StoreSDNode *, 8> ChainedStores = findChainedStores(STChain, BasePtr);
  if (ChainedStores.size() > 0) {
     mergeChainedStores(ChainedStores);
     return true;
  }

  // Improve St's Chain..
  SDValue BetterChain = FindBetterChain(St, St->getChain());
  if (St->getChain() != BetterChain) {
    replaceStoreChain(St, BetterChain);
    return true;
  }
  return false;
}
```

(maybe even merge `findChainedStores` into `mergeChainedStores`)


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18950
   // pointer.
   BaseIndexOffset BasePtr = BaseIndexOffset::match(St, DAG);
 
----------------
Please make this const to save the reader the trouble of tracking all uses.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18969
+  StoreSDNode *STChain = St;
+  int64_t STChainOffset = 0;
+  while (StoreSDNode *Chain = dyn_cast<StoreSDNode>(STChain->getChain())) {
----------------
this is unused


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18982
       break;
-
-    // Walk up the chain to find the next store node, ignoring any
-    // intermediate loads. Any other kind of node will halt the loop.
-    SDNode *NextInChain = Index->getChain().getNode();
-    while (true) {
-      if (StoreSDNode *STn = dyn_cast<StoreSDNode>(NextInChain)) {
-        // We found a store node. Use it for the next iteration.
-        if (STn->isVolatile() || STn->isIndexed()) {
-          Index = nullptr;
-          break;
-        }
-        ChainedStores.push_back(STn);
-        Index = STn;
+    // Make sure we're don't overlap.
+    int64_t Length = Chain->getMemoryVT().getSizeInBits() / 8;
----------------
we're -> we


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18984
+    int64_t Length = Chain->getMemoryVT().getSizeInBits() / 8;
+    auto Res = IntervalsCovered.insert(std::make_pair(Offset, Offset + Length));
+    // Must be valid.
----------------
What about using `llvm::IntervalMap<int64_t, int>` for `IntervalsCovered` ? It would simplify the code here.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:18987
+    auto I = Res.first;
+    if (!Res.second || I == IntervalsCovered.end())
+      break;
----------------
why are you checking the iterator (rhs) ? The first check should be enough.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19015
+    SmallVector<SDValue, 8> TFOps;
+    SDValue NewST;
+    bool AllImproved = true;
----------------
please move this closer to where it's used.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19055
+      TFOps.insert(TFOps.begin(), NewChain);
+    // TFOps.push_back(NewChain);
 
----------------
remove


Repository:
  rL LLVM

https://reviews.llvm.org/D53552