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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17088-17090
+  //      t0: ch = store
+  //    t1: ch = callseq_start t0,
+  // t2: ch = load t1,
----------------
This comment is misplaced. I think all this does is:
```
// Look through CALLSEQ_START.
```


================
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
----------------
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);
```
  



================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17098-17103
+    // Handle the cases like:
+    //        t0: ch = store
+    //        t1: ch = store
+    //      t2: ch = TokenFactor t0, t1
+    //    t3: ch = callseq_start t2,
+    // t4: ch = load t3,
----------------
Nit: this comment can simply be:
```
// Look for unique store within the TokenFactor.
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:17113-17114
+        GatherAllAliases(Store, Chain, Aliases);
+        if (Aliases.empty() ||
+            (Aliases.size() == 1 && Aliases[0].getNode() == Store))
+          ST = Store;
----------------
Nit:
```
if (Aliases.empty() || (Aliases.size() == 1 && Aliases.front() == Store)
```


================
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;
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138899