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

[Nirav Dave via Phabricator via llvm-commits]

niravd accepted this revision.
niravd added a comment.
This revision is now accepted and ready to land.

LGTM, modulo potentially excising the hasOneUse as I've suggested provided you agree with my line of reasoning there.



================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15397
+      break;
+    case ISD::LIFETIME_END:
+      Chains.push_back(Chain.getOperand(0));
----------------
courbet wrote:
> niravd wrote:
> > LIFETIME_START as well?
> Indeed, but only for `lifetime_start`s that do not overlap. I'm not checking the overlap for lifetime_end because you cannot have:
> 
> ```
> lifetime_end(a);
> lifetime_end(a);
> ```
> 
> but you might have:
> 
> ```
> lifetime_start(a);
> lifetime_end(a);
> lifetime_start(a); // 3
> lifetime_end(a);   // 4
> ```
> 
> and you don't want 4 to go past 3.
> 
Yes. Maybe add the check for LIFETIME_END as well, just as a sanity check. I worry that we may have a case where a and a' overlap.

lifetime_start(a);
lifetime_end(a);
lifetime_start(a'); // 3
lifetime_end(a');   // 4

But it's fine as is.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15402
+      StoreSDNode *ST = dyn_cast<StoreSDNode>(Chain);
+      if (ST->isVolatile() || !ST->hasOneUse() || ST->isIndexed())
+        continue;
----------------
courbet wrote:
> niravd wrote:
> > You can remove "!ST->hasOneUse()" now that you have the indexed check.
> I still need to check the `!hasOneUse()` check because the store might be depended on by several users.
Shoudn't the fact that that the store is an immediate (or at least only through TF) predecessor to the LIFETIME_END be sufficient proof that the store can be removed? In which case we should be able to remove it completely. 

The only way it'll fail the hasOneUse case is if we have some redundancy somewhere  (presumably because we gave up partway in our alias analysis). 

I don't expect to see this very often, so either way is probably fine.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15412
+                   LifetimeBaseIndex.dump(); dbgs() << "\n");
+        CombineTo(ST, ST->getChain());
+        return SDValue(N, 0);
----------------
courbet wrote:
> niravd wrote:
> > Nit: return CombineTo(ST, ST->getChain);
> Not sure I understand why ?
Oh right. You're returning LIFETIME_END. Nevermind.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGAddressAnalysis.cpp:90
+  int64_t PtrDiff;
+  if (BasePtr0.equalBaseIndex(BasePtr1, DAG, PtrDiff)) {
+    IsAlias = !((NumBytes0 <= PtrDiff) || (PtrDiff + NumBytes1 <= 0));
----------------
courbet wrote:
> niravd wrote:
> > This is "contains".
> Not exactly. `contains` means "one is within the other" (and is not symmetrical), here it's "one might overlap the other" (and is symmetrical). I've added graphical comments + the symmetric formula for reference.
Right!


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57541