[PATCH] D138899: [DAGCombiner] handle more store value forwarding

[ChenZheng via Phabricator via llvm-commits]

shchenz marked an inline comment as done.
shchenz added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17112
+        // Make sure the store is not aliased with any nodes in TokenFactor.
+        GatherAllAliases(Store, Chain, Aliases);
+        if (Aliases.empty() ||
----------------
RolandF wrote:
> Is this check really needed for token factor operands? They are supposed to be independent. Aliasing seems like an order dependence.
This is what I originally thought, then I met following case:
```
t16: ch = store<(store (s16) into @x1a0, !tbaa !8)> t15, t13, GlobalAddress:i64<ptr @x1a0> 0, undef:i64
  t18: i16,ch = load<(dereferenceable load (s16) from @x1a2, !tbaa !8)> t16, GlobalAddress:i64<ptr @x1a2> 0, undef:i64
  t20: i16,ch = load<(dereferenceable load (s16) from @i, !tbaa !8)> t16, GlobalAddress:i64<ptr @i> 0, undef:i64
  t25: i16,ch = load<(load (s16) from %ir.arrayidx.i, !tbaa !8)> t16, t24, undef:i64
  t27: i16,ch = load<(dereferenceable load (s16) from @si, !tbaa !8)> t16, GlobalAddress:i64<ptr @si> 0, undef:i64

    t37: ch = TokenFactor t18:1, t20:1, t25:1, t27:1

  t38: ch = store<(store (s16) into %ir.arrayidx.i, !tbaa !8)> t37, t36, t24, undef:i64

  t41: i16,ch = load<(dereferenceable load (s16) from @x1a0, !tbaa !8)> t38, GlobalAddress:i64<ptr @x1a0> 0, undef:i64
```

======>
```
  t16: ch = store<(store (s16) into @x1a0, !tbaa !8)> t15, t13, GlobalAddress:i64<ptr @x1a0> 0, undef:i64
  t18: i16,ch = load<(dereferenceable load (s16) from @x1a2, !tbaa !8)> t16, GlobalAddress:i64<ptr @x1a2> 0, undef:i64
  t20: i16,ch = load<(dereferenceable load (s16) from @i, !tbaa !8)> t16, GlobalAddress:i64<ptr @i> 0, undef:i64

  t25: i16,ch = load<(load (s16) from %ir.arrayidx.i, !tbaa !8)> t16, t24, undef:i64
  t27: i16,ch = load<(dereferenceable load (s16) from @si, !tbaa !8)> t16, GlobalAddress:i64<ptr @si> 0, undef:i64


    t915: ch = TokenFactor t16, t25:1
  t916: ch = store<(store (s16) into %ir.arrayidx.i, !tbaa !8)> t915, t36, t24, undef:i64
    t919: ch = TokenFactor t916, t16
  t920: i16,ch = load<(dereferenceable load (s16) from @x1a0, !tbaa !8)> t919, GlobalAddress:i64<ptr @x1a0> 0, undef:i64
```

DAGCombiner will forward the store value of `t16` to `t920` if no this aliasing check. But actually `t916`(`t38`) is aliased to `t16`, so the load result should be the content of `t916` instead of `t16`.

And from the implementation of `GatherAllAliases`, one operand in a `TokenFactor` node does not mean it is not aliased to other operands of the `TokenFactor` node. That's the reason why it checks aliasing by checking each child operand of the `TokenFactor` node?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138899