[PATCH] D57541: [DAGCombiner] Eliminate dead stores to stack.

[Nirav Dave via Phabricator via llvm-commits]

niravd added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15386
+
+  // We walk up the chains to find stores.
+  SmallVector<SDValue, 8> Chains = {N->getOperand(0)};
----------------
courbet wrote:
> niravd wrote:
> > I think we should merge this with FindBetterChains. At the least we should have load/store aliasing be able to bypass unrelated lifetime nodes and vice versa.
> I'm not exactly sure how to merge it, but teaching it to bypass unrelated lifetime nodes sounds like a good idea. Done.
I was thinking of this as two combines: One sharing logic with FindBetterChains to improve LIFETIME_END's chain to simplify it's chain to (presumably just the store chain) and the other to check the chain of a LIFETIME_END like we do with dead stores via overlapping stores. You already have the second.





================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15397
+      break;
+    case ISD::LIFETIME_END:
+      Chains.push_back(Chain.getOperand(0));
----------------
LIFETIME_START as well?


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15402
+      StoreSDNode *ST = dyn_cast<StoreSDNode>(Chain);
+      if (ST->isVolatile() || !ST->hasOneUse() || ST->isIndexed())
+        continue;
----------------
You can remove "!ST->hasOneUse()" now that you have the indexed check.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15412
+                   LifetimeBaseIndex.dump(); dbgs() << "\n");
+        CombineTo(ST, ST->getChain());
+        return SDValue(N, 0);
----------------
Nit: return CombineTo(ST, ST->getChain);


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:90
+  int64_t PtrDiff;
+  if (BasePtr0.equalBaseIndex(BasePtr1, DAG, PtrDiff)) {
+    IsAlias = !((NumBytes0 <= PtrDiff) || (PtrDiff + NumBytes1 <= 0));
----------------
This is "contains".


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57541/new/

https://reviews.llvm.org/D57541