[PATCH] D142100: [DAGCombine] Allow scalable type dead store elimination

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19807
   if (StoreSDNode *ST1 = dyn_cast<StoreSDNode>(Chain)) {
-    if (ST->isUnindexed() && ST->isSimple() &&
-        ST1->isUnindexed() && ST1->isSimple()) {
+    if (ST->isUnindexed() && ST->isSimple() && ST1->isUnindexed() &&
+        ST1->isSimple()) {
----------------
nit: unintended change?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:19819
           !ST1->getBasePtr().isUndef() &&
-          // BaseIndexOffset and the code below requires knowing the size
-          // of a vector, so bail out if MemoryVT is scalable.
-          !ST->getMemoryVT().isScalableVector() &&
-          !ST1->getMemoryVT().isScalableVector() &&
           ST->getAddressSpace() == ST1->getAddressSpace()) {
+        // If we consider two stores and one smaller in size is a scalable
----------------
I wonder if it's simpler to write this as follows:

  if (ST->getMemoryVT().isScalableVector() ||
      ST1->getMemoryVT().isScalableVector()) {
    if (ST1->getBasePtr() == Ptr && TypeSize::isKnownLE(...))
      CombineTo(ST1, ST1->getChain());
  } else {
    const BaseIndexOffset STBase = BaseIndexOffset::match(ST, DAG);
    const BaseIndexOffset ChainBase = BaseIndexOffset::match(ST1, DAG);
    if (STBase.contains(...))
      CombineTo(ST1, ST1->getChain());
  }

That way the the logic between 'anything scalable' and everything else is clearly separated.


================
Comment at: llvm/test/CodeGen/AArch64/sve-redundant-store.ll:21
 }
+
+; make sure that scalable store is present, becuase we don't know its final size.
----------------
This is missing some test cases, e.g.
* One where both stores are scalable, and same size.
* One where both stores are scalable, and second store is larger
* One where both stores are scalable, and first store is larger (negative test)


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

https://reviews.llvm.org/D142100