[PATCH] D49200: [DAGCombine] Improve Load-Store Forwarding

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12377-12382
   // If this load is directly stored, replace the load value with the stored
   // value.
-  // TODO: Handle store large -> read small portion.
-  // TODO: Handle TRUNCSTORE/LOADEXT
-  if (OptLevel != CodeGenOpt::None &&
-      ISD::isNormalLoad(N) && !LD->isVolatile()) {
-    if (ISD::isNON_TRUNCStore(Chain.getNode())) {
-      StoreSDNode *PrevST = cast<StoreSDNode>(Chain);
-      if (PrevST->getBasePtr() == Ptr &&
-          PrevST->getValue().getValueType() == N->getValueType(0))
-        return CombineTo(N, PrevST->getOperand(1), Chain);
+  StoreSDNode *ST = dyn_cast<StoreSDNode>(Chain.getNode());
+  if (OptLevel != CodeGenOpt::None && !LD->isVolatile() && ST &&
+      !ST->isVolatile()) {
+    auto LDType = LD->getValueType(0);
----------------
This whole `if` looks like it should be its own helper, and even then it could be broken up more.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12382-12385
+    auto LDType = LD->getValueType(0);
+    auto LDMemType = LD->getMemoryVT();
+    auto STMemType = ST->getMemoryVT();
+    auto STType = ST->getValue().getValueType();
----------------
This use of auto is not consistent with LLVM's coding standards:
https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12417
+    do {
+      // TODO" Deal with non-zero offsets
+      if (LD->getBasePtr().isUndef() || Offset != 0 || !STCoversLD)
----------------
`TODO"` seems like a typo


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12421-12424
+      if (STType == STMemType) { // Do nothing/
+      } else if (!isTypeLegal(STMemType))
+        break; // fail.
+      else if (STType.isFloatingPoint() && STMemType.isFloatingPoint() &&
----------------
These conditionals look like they can be simplified.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:12449-12450
+      } else if (LDMemType.isInteger() && LDType.isInteger())
+        switch (LD->getExtensionType()) {
+        case ISD::NON_EXTLOAD:
+          Val = DAG.getBitcast(LDType, Val);
----------------
This switch could be its own helper function, `extendingLoadToExtension`, or something like that. Putting switches into helpers is nice since it can save you a local variable and saves the `break;` line in each case.


Repository:
  rL LLVM

https://reviews.llvm.org/D49200