[PATCH] D143069: [DAGCombine] Allow DAGCombine to remove dead masked stores

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11167
+    if (MST->isUnindexed() && MST->isSimple() && MST1->isUnindexed() &&
+        MST1->isSimple() && OptLevel != CodeGenOpt::None &&
+        MST1->getBasePtr() == Ptr && MST1->hasOneUse() &&
----------------
Is this check necessary? I would expect that it doesn't run the DAGCombiner when OptLevel == CodeGenOpt::None. Does it still run the DAGCombiner with -O0?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11168
+        MST1->isSimple() && OptLevel != CodeGenOpt::None &&
+        MST1->getBasePtr() == Ptr && MST1->hasOneUse() &&
+        !MST1->getBasePtr().isUndef() &&
----------------
A store returns a Chain, which always has one use, so this test can be removed.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11169
+        MST1->getBasePtr() == Ptr && MST1->hasOneUse() &&
+        !MST1->getBasePtr().isUndef() &&
+        MST->getAddressSpace() == MST1->getAddressSpace() &&
----------------
This seems like an odd case to ever happen, and you did not add a specific test for this case. Why did you add this check?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11170
+        !MST1->getBasePtr().isUndef() &&
+        MST->getAddressSpace() == MST1->getAddressSpace() &&
+        Mask == MST1->getMask() &&
----------------
Is this check necessary? I would expect that if both pointers are equal, they use the same address space?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:11172
+        Mask == MST1->getMask() &&
+        TypeSize::isKnownLE(MST1->getMemoryVT().getStoreSize(),
+                            MST->getMemoryVT().getStoreSize())) {
----------------
I think this can be isKnownEQ, because for the predicates to match, the sizes must match too.


================
Comment at: llvm/test/CodeGen/AArch64/sve-dead-masked-store.ll:12
+  ret void
+}
+declare void @llvm.masked.store.nxv4i32(<vscale x 4 x i32>, <vscale x 4 x i32>*, i32, <vscale x 4 x i1>)
----------------
Another case that can be handled is where the second store has an 'all true' mask, then it doesn't really matter what the mask of the first store is, e.g.

  %alltrue.ins = insertelement <vscale x 4 x i1> poison, i1 true, i32 0
  %alltrue = shufflevector  <vscale x 4 x i1> %splat.ins,  <vscale x 4 x i1> poison,  <vscale x 4 x i32> zeroinitializer
  call void @llvm.masked.store.nxv4i32(<vscale x 4 x i32> %val, ptr %a, i32 4, <vscale x 4 x i1> %mask)
  call void @llvm.masked.store.nxv4i32(<vscale x 4 x i32> %val, ptr %a, i32 4, <vscale x 4 x i1> %alltrue)



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

https://reviews.llvm.org/D143069