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

[Clement Courbet via Phabricator via llvm-commits]

courbet marked 2 inline comments as done.
courbet added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:15402
+      StoreSDNode *ST = dyn_cast<StoreSDNode>(Chain);
+      if (ST->isVolatile() || !ST->hasOneUse() || ST->isIndexed())
+        continue;
----------------
niravd wrote:
> 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.
> 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.

Consider the example in test `onealloc_readback_1`. During the combine, just before we visit t23 in the dag looks like this:

```
SelectionDAG has 26 nodes:
  t0: ch = EntryToken
  t2: i64,ch = CopyFromReg t0, Register:i64 %0
    t9: ch = lifetime.start<16 to 32> t0, TargetFrameIndex:i64<0>
  t10: ch = lifetime.start<0 to 16> t9, TargetFrameIndex:i64<0>
  t11: i64 = Constant<0>
  t13: v16i8,ch = load<(load 16 from %ir.a, align 1)> t10, t2, undef:i64
      t7: i64 = add nuw FrameIndex:i64<0>, Constant:i64<16>
    t14: ch = store<(store 16 into %ir.part1, align 1)> t10, t13, t7, undef:i64
  t15: ch = TokenFactor t13:1, t14
    t4: i64,ch = CopyFromReg t0, Register:i64 %1
  t16: v16i8,ch = load<(load 16 from %ir.b, align 1)> t15, t4, undef:i64
  t17: ch = store<(store 16 into %ir.part21)> t15, t16, FrameIndex:i64<0>, undef:i64
  t28: v16i8,ch = load<(load 16 from %ir.part21)> t17, FrameIndex:i64<0>, undef:i64
          t18: ch = TokenFactor t16:1, t17
        t19: ch = lifetime.end<16 to 32> t18, TargetFrameIndex:i64<0>
        t26: ch = store<(store 16 into %ir.a, align 1)> t0, t28, t2, undef:i64
      t30: ch = TokenFactor t19, t28:1, t26
    t23: ch = lifetime.end<0 to 16> t30, TargetFrameIndex:i64<0>
  t25: ch = X86ISD::RET_FLAG t23, TargetConstant:i32<0>
```

Note how the store `t17` is in immediate predecessor (modulo TF & non-aliasing lifetime end) of lifetime end `t23`. However, load `t28` also depends on `t17`. But the lifetime end `t23` does not care about the load, so it makes sense for `t23` not to have a chain on `t28`.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D57541