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

[Roland Froese via Phabricator via llvm-commits]

RolandF added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17107
+      if (!Store)
+        continue;
+      BaseIndexOffset BasePtrLD = BaseIndexOffset::match(LD, DAG);
----------------
If we can't make the assumption that token factor operands are unaliased, I think that the non-store path needs to be checked more strongly.  There are ways to touch memory besides stores.  I would suggest to whitelist the kinds of operands we know are safe and to otherwise give up.


================
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() ||
----------------
shchenz wrote:
> 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?
ok


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138899