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

[ChenZheng via Phabricator via llvm-commits]

shchenz added a comment.

Thanks for review @nemanjai .



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17094-17128
+  int64_t Offset;
+  StoreSDNode *ST = nullptr;
+  SmallVector<SDValue, 8> Aliases;
+  if (Chain.getOpcode() == ISD::TokenFactor) {
+    // Handle the cases like:
+    //        t0: ch = store
+    //        t1: ch = store
----------------
nemanjai wrote:
> I think it would be better for readability if you put this into a `static` function. Something like:
> ```
> // Find a unique store that feeds this load if such a store exists.
> // This will look through CALLSEQ_START to allow forwarding
> // stores to the stack for byval arguments.
> static StoreSDNode *getUniqueStoreFeeding(LoadSDNode *Load, SelectionDAG &DAG);
> ```
>   
> 
Done. Seems I have to use a member function as `GatherAllAliases` is a member function.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17122-17125
+      BaseIndexOffset BasePtrLD = BaseIndexOffset::match(LD, DAG);
+      BaseIndexOffset BasePtrST = BaseIndexOffset::match(Store, DAG);
+      if (BasePtrST.equalBaseIndex(BasePtrLD, DAG, Offset))
+        ST = Store;
----------------
nemanjai wrote:
> Nit: rather than repeating this code, it might aid readability if you collect all unaliased stores (either the only store chained to CALLSEQ_START or all the unaliased stores in the TokenFactor) and then look for one with a matching `BaseIndexOffset`.
Thanks, I get your point. A function that finds all unaliased stores in `TokenFactor` may be time consuming. And we don't just care about store nodes, we care all nodes that write memory. So to me finding the candidate store first and then check alias with memory nodes in `TokenFactor` with the specific candidate store can save some compile time?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138899